8000 Allow scikit-learn 1.3 by markotoplak · Pull Request #6585 · biolab/orange3 · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@markotoplak
Copy link
Member
@markotoplak markotoplak commented Sep 21, 2023

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 whether fit() 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.

@markotoplak markotoplak marked this pull request as draft September 21, 2023 20:42
@markotoplak markotoplak marked this pull request as ready for review September 25, 2023 07:15
@markotoplak
Copy link
Member Author

The most of this PR is adapting a hack from 2016 to scikit-learn 1.3.

@noahnovsak
Copy link
Contributor

The fix seems fine. I feel like a warning or deadline would be nice though, to scare us into hardcoding the values instead.

@markotoplak
Copy link
Member Author

@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.

pyyaml
requests
scikit-learn>=1.1.0,<1.2.0
scikit-learn>=1.1.0,!=1.2.* # ignoring 1.2.*: scikit-learn/issues/26241
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor
@PrimozGodec PrimozGodec left a 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.

@markotoplak
Copy link
Member Author

@PrimozGodec, I was betting on you to complain that the deprecation does not have an associated time bomb. :)

@PrimozGodec
Copy link
Contributor
PrimozGodec commented Sep 29, 2023

@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. :)

@PrimozGodec PrimozGodec merged commit 0f301bd into biolab:master Sep 29, 2023
@markotoplak markotoplak deleted the allow-scikit-1.3 branch November 6, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0