MLJ extension

[tjjarvinen]

This PR adds support for MLJ interface solvers.

At this state there is only support for MLJScikitLearnInterface.jl and MLJLinearModels.jl. You can look at all available MLJ solvers here.

To use MLJ you just need to create MLJ solver and then use it with linear_fit like any other solver

using ACEfit
using MLJ

# Load ARD solver
ARDRegressor = @load ARDRegressor pkg=MLJScikitLearnInterface

# Create the solver itself and give it parameters
solver = ARDRegressor(
    n_iter = 300,
    tol = 1e-3,
    threshold_lambda = 10000
)

# This is equal to the already existing ARD solver we have

[cortner]

questions:

• the docs say it but can you please still add a line to the code snipped to explain how the solvers are used and clarify that the ACEfit.solve is overloaded to be able to use those MLJ solvers.
• why are there multiple interfaces? I don't understand why there can't be a single interface to MLJ?
• does this fully replace the current SKLEARN interface?

[cortner]

@wcwitt -- we thought it would be useful to try out this interface as there are a lot of solvers in MLJ that we haven't considered yet. It should really change anything a bit a strict extension. Btw - once you spin your BLR solvers out of ACEfit you may want to consider making then accessible via MLJ as well.

There is also more functionality in MLJ that we should consider incorporating into our workflow rather than rewrite things from scratch.

[tjjarvinen]

• why are there multiple interfaces? I don't understand why there can't be a single interface to MLJ?

MLJ has different output depending on solvers. SKLearn solvers e.g. return Python objects, while MLJLinearModels returns Julia Vectors. You will then need to transform the output to our format, so I just found it easier to make two extensions.

• does this fully replace the current SKLEARN interface?

It is not a replacement, but an addition. Nothing has been removed and everything works like in the past. Now we just have two alternative ways to use SKLearn.

[wcwitt]

Thanks both, I'm supportive of this.

does this fully replace the current SKLEARN interface?

It is not a replacement, but an addition. Nothing has been removed and everything works like in the past. Now we just have two alternative ways to use SKLearn.

But could we go ahead and remove the old way?

[wcwitt]

I have a general question about J1.9 extensions - how should we decide when to use them?

My (weakly held) preference is to keep them to a minimum. It makes sense to use them here because of the Python dependencies, but how do you think about the decision in general?

[cortner]

I agree, keeping them to a minimum. I would use them for heavy dependencies that take a long time to load or that are rarely used

[tjjarvinen]

But could we go ahead and remove the old way?

We could. But MLJ SKLearn has some issues, like we cannot get the number of iterations the solver used. So, for now I would take it as "a testing status" and then later on when it has been in use for some time, we can drop the old one.

[tjjarvinen]

I agree, keeping them to a minimum. I would use them for heavy dependencies that take a long time to load or that are rarely used

This is the way to go.

[cortner]

Regarding the old sklearn interface how about we @deprecate it?

[tjjarvinen]

Extensions complicate @deprecate-macro use. We could deprecate the old SKLearn structure, but we could not give any new structure, as an new(x) for the macro because of extensions.

Maybe just adding a @warn there that would give instruction of the MLJ use be a better option?

[cortner]

I see - a warning sounds good then. And remove at the next minor version.

[cortner]

I wouldn't call it "better" just that this will be removed in favour of the MLJ interface.

[cortner]

is this tentatively ready to review and merge?

[cortner]

Just to note here #64 -- I don't propose to extend this PR but just to be aware that other parts of the MLJ ecosystem could be very useful for us and we should consider integrating them as well.

[tjjarvinen]

Yes, it should be in working condition now. Next step would be to gather user experience.

[cortner]

@wcwitt if this doesn't change functionality, should we merge and register and treat any problems as future bugfixes? Are you willing to take a quick look at it?

[wcwitt]

Sounds fine, running out of time to look it over today, but I will by tomorrow.

Edit: might need another day, sorry.

[wcwitt]

Thanks for adding these features. I made two small comments.

[wcwitt]

Conceptually, does it make sense to recommend that MLJ is loaded first?

[tjjarvinen]

Loading order does not matter, so I would leave it as it is.

[wcwitt]

The example in ext/ACEfit_MLJLinearModels_ext.jl loads MLJ first - can we make them consistent?

[wcwitt]

I believe this line should be deleted?

[tjjarvinen]

MLJ has different back ends. MLJLinearModels and MLJScikitLearnInterface are the ones that have been implemented. The point here is that these two behave differently on the background, so we need to test them both. So, there is one test for MLJScikitLearnInterface and one for MLJLinearModels. So, removing the part you suggest would leave MLJLinearModels without a test.

We could put here some solver that would be actually used, but all MLJLinearModels have the same interface on the background, so I thought that testing with LSQR would be the fastest.

[wcwitt]

Hm, maybe I don't understand what's happening then. But it looks like we set solver equal to LinearRegressor() and then immediately overwrite it with a non-MLJ LSQR in the next line. Is that wrong?

By contrast the MLJScikitLearnInterface test makes sense to me.

[tjjarvinen]

Nope, you are correct and I am the one who cannot read...

Fixed the test and added the needed fix for parameter extraction.

[wcwitt]

@cortner I just merged, not sure if you want to tag at this stage or not

[cortner]

I like tagging many versions. This is now registered as 0.1.3.