ACEsuit / ACEfit.jl

Generic Codes for Fitting ACE models
MIT License
7 stars 6 forks source link

MLJ extension #61

Closed tjjarvinen closed 1 year ago

tjjarvinen commented 1 year ago

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 commented 1 year ago

questions:

cortner commented 1 year ago

@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 commented 1 year ago
  • 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

tjjarvinen commented 1 year ago

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 commented 1 year ago

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

cortner commented 1 year ago

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

cortner commented 1 year ago

is this tentatively ready to review and merge?

cortner commented 1 year ago

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 commented 1 year ago

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

cortner commented 1 year ago

@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 commented 1 year ago

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

Edit: might need another day, sorry.

wcwitt commented 1 year ago

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

cortner commented 1 year ago

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