-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow scikit-learn 1.3 #6585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow scikit-learn 1.3 #6585
Conversation
9fe5410 to
8646add
Compare
scikit-learn 1.3 fixes a bug introduced by AdaBoost deprecations in 1.2 More: scikit-learn/scikit-learn#26241
bb907f2 to
e95c041
Compare
|
The most of this PR is adapting a hack from 2016 to scikit-learn 1.3. |
|
The fix seems fine. I feel like a warning or deadline would be nice though, to scare us into hardcoding the values instead. |
|
@noahnovsak, if we decide against this magic, then the way to do it would be to deprecate the baseclass property with OrangeDeprecationWarning and manually set it as an attribute in derived classes. Then, if tests still pass, we are happy. No need for deadlines. |
7d4de51 to
0c23747
Compare
requirements-core.txt
Outdated
| pyyaml | ||
| requests | ||
| scikit-learn>=1.1.0,<1.2.0 | ||
| scikit-learn>=1.1.0,!=1.2.* # ignoring 1.2.*: scikit-learn/issues/26241 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any specific reason why meta.yaml includes <1.4 version scikit-learn constraint while core doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. 1.4 deprecates some things that are still used but unfixed by this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides a comment it looks good to me.
|
@PrimozGodec, I was betting on you to complain that the deprecation does not have an associated time bomb. :) |
Yep, haven't thought about that before, but it would be nice. :) |
0c23747 to
e9b2a16
Compare
scikit-learn 1.3 fixes a bug introduced by AdaBoost deprecations in 1.2
Bug: scikit-learn/scikit-learn#26241
Fix was released in 1.3 (from changelog):
Fix deprecation of base_estimator in ensemble.AdaBoostClassifier and ensemble.AdaBoostRegressor that was introduced in #23819. #26242 by Marko Toplak.
But 1.3 also introduced some changes to the
fit()internals, therefore old "magic" that was supposed to find out whetherfit()supports the weights too stopped working. I adapted what we had to 1.3 and also wrote some tests for it. I think the best way to go forward would actually be to remove that magic and hardcode it per-learner.