Skip to content
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

Update start_iteration_callback to accept pipeline instance instead of pipeline class #2290

Merged
merged 10 commits into from May 20, 2021

Conversation

angela97lin
Copy link
Contributor

Closes #2282

@angela97lin angela97lin self-assigned this May 19, 2021
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #2290 (3f93f0c) into main (8a803d4) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #2290     +/-   ##
=========================================
- Coverage   100.0%   100.0%   -0.0%     
=========================================
  Files         280      280             
  Lines       24410    24396     -14     
=========================================
- Hits        24387    24373     -14     
  Misses         23       23             
Impacted Files Coverage Δ
evalml/automl/automl_search.py 100.0% <100.0%> (ø)
evalml/tests/automl_tests/test_automl.py 99.7% <100.0%> (ø)
.../automl_tests/test_automl_search_classification.py 100.0% <100.0%> (ø)
...ests/automl_tests/test_automl_search_regression.py 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a803d4...3f93f0c. Read the comment docs.


def test_automl_regression_nonlinear_pipeline_search(nonlinear_regression_pipeline_class, X_y_regression):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combined :)


def test_automl_multiclass_nonlinear_pipeline_search_more_iterations(nonlinear_multiclass_pipeline_class, X_y_multi):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of confused by what this test was testing (that multiclass with more iterations will produce the right pipelines..?), but combined with above :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Yeah looking at this now, I'm not sure either haha. It feels like it could be an automl algo test which we put in automl search before those concepts had separation. Idk!

@angela97lin angela97lin marked this pull request as ready for review May 20, 2021 04:04
Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I had two small changes to request: I think we should delete the parameters argument, and let's list this in breaking changes in the release notes.

@@ -7,6 +7,7 @@ Release Notes
* Changed the default parameter values for ``Elastic Net Classifier`` and ``Elastic Net Regressor`` :pr:`2269`
* Fixes
* Changes
* Updated ``start_iteration_callback`` to accept a pipeline instance instead of a pipeline class :pr:`2290`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a nit-pick, but technically we should add a breaking change entry too, because this is going out in a different release from the pipeline API change.


def test_automl_multiclass_nonlinear_pipeline_search_more_iterations(nonlinear_multiclass_pipeline_class, X_y_multi):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Yeah looking at this now, I'm not sure either haha. It feels like it could be an automl algo test which we put in automl search before those concepts had separation. Idk!

assert start_iteration_callback.call_args_list[4][0][0] == nonlinear_multiclass_pipeline_class
assert isinstance(start_iteration_callback.call_args_list[0][0][0], expected_mock_class)
for i in range(1, 5):
assert isinstance(start_iteration_callback.call_args_list[i][0][0], pipeline_class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -450,7 +450,7 @@ def _get_batch_number(self):

def _pre_evaluation_callback(self, pipeline):
if self.start_iteration_callback:
self.start_iteration_callback(pipeline.__class__, pipeline.parameters, self)
self.start_iteration_callback(pipeline, pipeline.parameters, self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're making a breaking change here: I think we should delete the parameters argument.

Why: now that we're providing a pipeline instance, the start iteration callback can access the parameters by calling pipeline.parameters, so there's no need for that to also be provided as a standalone arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angela97lin Looks good! Don't have much to add beyond @dsherry 's comments.

@angela97lin angela97lin dismissed dsherry’s stale review May 20, 2021 18:13

Changes addressed

@angela97lin angela97lin requested a review from dsherry May 20, 2021 18:13
Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 😁

@angela97lin angela97lin merged commit a66ea32 into main May 20, 2021
@angela97lin angela97lin deleted the 2282_pipeline_callback branch May 20, 2021 18:30
@chukarsten chukarsten mentioned this pull request May 24, 2021
@chukarsten chukarsten mentioned this pull request Jun 2, 2021
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.

AutoML search: start_iteration_callback is passed pipeline class instead of pipeline instance
3 participants