MNT Sklearn1.6 compatibility

[TamaraAtanasoska]

Reference Issues/PRs

Fixes #443. Not fully working yet.

What does this implement/fix? Explain your changes.

A few of the compatibility errors, including the one explained in #443 are now fixed. There are some external failures(errors happening outside of skops) that I am not sure how to fix. @adrinjalali any tips? Below are the remaining failures:

FAILED skops/io/tests/test_persist.py::test_can_persist_fitted[GraphicalLassoCV(cv=3,max_iter=5)] - FloatingPointError: Non SPD result: the system is too ill-conditioned for this solver. The system is too ill-conditioned for this ...
FAILED skops/io/tests/test_persist.py::test_can_persist_fitted[PassiveAggressiveClassifier(max_iter=5)] - AssertionError
FAILED skops/io/tests/test_persist.py::test_can_persist_fitted[SGDClassifier(max_iter=5)] - AssertionError
FAILED skops/io/tests/test_persist.py::test_can_persist_fitted[SGDOneClassSVM(max_iter=5)] - AssertionError

Any other comments?

First PR to the project, the fixes might be off.
I understood #443 as I can remove all SGD stuff from the _sklearn.py file when working with sklearn 1.6+, tests pass like that, I am just not sure if the change is supposed to be so severe?

[TamaraAtanasoska]

Issue in quantile_forest made: zillow/quantile-forest#103

[adrinjalali]

Can you get the output with pytest -l to see all the local variables on the stack trace?

[TamaraAtanasoska]

Can you get the output with pytest -l to see all the local variables on the stack trace?

The issue is about versioning again, taking care of separating the SGD stuff by sklearn version would most likely solve these last errors. I will look into it today, seems a bit more complex in this case then the rest, at least at first glance. I will ping with questions.

[adrinjalali]

This would pass tests once scikit-learn/scikit-learn#30356 is merged in sklearn

[adrinjalali]

scikit-learn now uses dataclasses for tags, which are emulated here for old sklearn as well

[adrinjalali]

scikit-learn has moved to a more central place for loss functions / classes, and therefore the ones which are used between estimators are removed from the sgd specific file.

[adrinjalali]

these are the new loss classes

[adrinjalali]

I've added the path argument to give the devs a better verbose idea of where things fail, to debug things easier

[adrinjalali]

an SGD estimator was ill defined with 50 samples, increasing to 100

[TamaraAtanasoska]

@adrinjalali any legwork left that you'd like to or can share to make this faster or it is good to go as it is when the changes are merged in sklearn?

[adrinjalali]

@TamaraAtanasoska what's left is to take all estimators generated by _construct_instances instead of only the first one. We'd need to restructure the test to allow for a for loop over the values yielded by the _construct_instances function.

[TamaraAtanasoska]

@TamaraAtanasoska what's left is to take all estimators generated by _construct_instances instead of only the first one. We'd need to restructure the test to allow for a for loop over the values yielded by the _construct_instances function.

do you mean in _tested_estimators and _unsupported_estimators? aren't both tests already looping through each estimator in all_estimators and then only creating an instance of it? or this exactly is the issue, it would be better to create instances of all estimators at once and then loop through them for sklearn 1.6?

Edit: I see this not about all_estimators, but all estimators produced by _construct_instances from that one estimator retrieved from all_estimators passed to it in the test 😄 it is confusing.

Should I take it on or you are already working on it?

[adrinjalali]

Should I take it on or you are already working on it?

No I'm not working on that, you can take it on :)

[adrinjalali]

BTW, we're working with @glemaitre on this: https://github.com/glemaitre/sklearn-compat

[TamaraAtanasoska]

BTW, we're working with @glemaitre on this: https://github.com/glemaitre/sklearn-compat

that is a cool effort! This can get stressful for a lot of folks otherwise

[TamaraAtanasoska]

@adrinjalali there is a TypeError related to the losses as a result of this new commit 712cb13. I suppose that will be fixed with everything together?

FAILED skops/io/tests/test_persist.py::test_can_persist_fitted[TweedieRegressor(max_iter=5)] - TypeError: __init__() takes exactly 1 positional argument (3 given)

>   ???
E   TypeError: __init__() takes exactly 1 positional argument (3 given)

sklearn/_loss/_loss.pyx:1615: TypeError
`

[adrinjalali]

Yes, that's exactly the error fixed with the now merged PR. The CI here should be green tomorrow once the nightly build is uploaded / updated.

[TamaraAtanasoska]

@adrinjalali refactor in c3da1b9 with the new changes regarding scikit-learn/scikit-learn#30372 although I guess that will be merged soon. If it gets merged before we merge I'd like to change the code slightly as I had it before as it was nicer :)

[adrinjalali]

There's a whole bunch of fixes here. I think I rather merge this and do other fixes in separate PRs.

Also, it turns out we were NOT testing against old sklearn versions, which is fixed now here, but it makes CI fail on those old ones, but at least they're tested. Also, the nightly build should be fixed (at least I tested locally with a latest version).