-
Notifications
You must be signed in to change notification settings - Fork 86
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
Updating scikit-learn and scikit-optimize to latest version #1141
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1141 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 196 196
Lines 11999 12006 +7
=======================================
+ Hits 11990 11997 +7
Misses 9 9
Continue to review full report at Codecov.
|
Performance tests forthcoming |
d796b4d
to
2160637
Compare
Performance Test Results here. Scores stayed the same but the fit times have increased slightly. I think it's still ok to merge this. |
2160637
to
a96b34a
Compare
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.
LGTM!
70dac87
to
384a765
Compare
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.
Thank you for tackling this!!
I left a question about the tests but that's all :)
scikit-learn>=0.21.3,!=0.22,<0.23.0 | ||
scikit-optimize>=0.7,<=0.7.4 | ||
scikit-learn>=0.23 | ||
scikit-optimize>=0.8 |
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.
What a relief! 😁 This was getting ugly.
@@ -199,10 +200,11 @@ class MockRegressor(Estimator): | |||
name = "Mock Regressor" | |||
model_family = ModelFamily.NONE | |||
supported_problem_types = [ProblemTypes.REGRESSION] | |||
hyperparameter_ranges = {} | |||
hyperparameter_ranges = {'a': Integer(0, 10), | |||
'b': Real(0, 10)} |
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.
@freddyaboulton I have the same question as @bchen1116 . You mentioned
the latest version of scikit-optimize wont optimize a pipeline without parameters
So which test(s) does setting these ranges affect? I don't think I follow yet.
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.
In test_automl_search_classification.py
and test_automl_search_regression.py
we pass in dummy_regression_pipeline_class
, dummy_binary_pipeline_class
, or dummy_multiclass_pipeline_class
as the allowed_pipelines
parameter to AutoMLSearch. Since these pipelines don't have parameters (because the estimator doesn't have any parameters) the SKOpt
tuner can't propose any pipelines and the search fails. Since we mock the pipeline fit and score methods, adding these parameters doesn't change the behavior of the tests.
The other option is to mock SKOptTuner.propose
in all tests that would fail but that seemed like a bigger change.
e0eba46
to
db9ad97
Compare
…o increase coverage.
db9ad97
to
478a23b
Compare
Pull Request Description
Fixes #1121 . With the release of
scikit-optimize
0.8.0, we can update toscikit-learn
version >= 0.23 (Release Notes).The only catch is that now
SKOptTuner
can't propose a new pipeline if the pipeline has no parameters. I had to add dummy parameters to our estimator fixtures to address this.We may want to add a check to
AutoML
that verifies the pipelines specified inallowed_pipelines
have parameters. This can be done in #454 or in a separate issue.After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.