Remove MLJ from testing by adding local Standardizer

lalvim / PartialLeastSquaresRegressor.jl

Implementation of a Partial Least Squares Regressor

MIT License

40 stars 8 forks source link

Remove MLJ from testing by adding local Standardizer #23

Closed ablaom closed 3 years ago

ablaom commented 3 years ago

Replaces #22

This PR:

extends the compatibility of MLJModelInterface to include latest version 0.4
replaces use of MLJ in testing with MLJBase wherever possible
replaces use of Standardizer (and the now redundant UnivariateStandardizer) from MLJModels with a local version defined in test/_standardizer.jl This eliminates some circular dependency issues - including PartialLeastSquaures in the model registry, which currently lives in the same place as MLJModels, which contains Standardizer

ablaom commented 3 years ago

@filipebraida @lalvim

filipebraida commented 3 years ago

Sorry for the delay to answer you (classes started last week). I didn't understand about Standardizer. Wouldn't it be better to remove it from the package and let the user define the pipeline?

ablaom commented 3 years ago

The point is that you use Standardizer, defined in MLJModels, for testing, which means it must be a test dependency of PartialLeastSquaresRegressor.jl. (Currently you have MLJ, which is one-higher up the food chain). The problem is that the model registry currently lives in MLJModels. So I have a circular dependency issue: want to update PLSR but this requires MLJModels, but I can't update MLJModels without updating the registry, which requires PLSR to be updated.

There may be other solutions, but from our end this is the easiest. I simply copied code for the Standardizer model you need for testing into your repo so you can use it independent of MLJModels (and MLJ). Your dependency for testing is then just MLJBase.

Is this clear?

ablaom commented 3 years ago

@filipebraida Just a note that users of the latest MLJ 0.16.0 are now unable to use your models. The compatibility needs to be extended, eg by merging this PR.

Note, to be clear, all this PR does is extend compatibility and add code to the testing suite to make current tests independent of MLJModels, MLJ. It does not change the existing tests.

filipebraida commented 3 years ago

@ablaom I asked to @lalvim to revise the code. As he implemented it, I think he better take a better look.

lalvim commented 3 years ago

@ablaom nice work.

I didn't really understand the point of the cause of circular dependency, but I believe it. I checked your codes and it looks ok. I did the merge. One question that remains is regarding the use outside the tests. Couldn't you use the mlj standardizer more?

ablaom commented 3 years ago

@lalvim @filipebraida Thanks for being accommodating.

One question that remains is regarding the use outside the tests. Couldn't you use the mlj standardizer more?

Outside of testing of this package, you can use Standardizer all you want.