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

Remove max_pipelines parameter from AutoMLSearch #1264

Merged
merged 16 commits into from Oct 21, 2020

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Oct 5, 2020

Closes #1201

Codecov fails because there are some lines in automl_search.py that are not tested and I'm decreasing the total number of lines in the file.

@angela97lin angela97lin self-assigned this Oct 5, 2020
@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

Merging #1264 into main will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1264      +/-   ##
==========================================
- Coverage   99.94%   99.94%   -0.01%     
==========================================
  Files         213      213              
  Lines       13436    13425      -11     
==========================================
- Hits        13429    13418      -11     
  Misses          7        7              
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.60% <ø> (-0.01%) ⬇️
evalml/tests/automl_tests/test_automl.py 100.00% <ø> (ø)
.../automl_tests/test_automl_search_classification.py 100.00% <ø> (ø)

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 1e4307a...08199d4. Read the comment docs.

@angela97lin angela97lin marked this pull request as ready for review October 6, 2020 15:18
dsherry
dsherry approved these changes Oct 7, 2020
Copy link
Collaborator

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

Thank you!

.. warning::

**Breaking Changes**
* ``max_pipelines`` parameter has been removed from ``AutoMLSearch``. Please use ``max_iterations`` instead. :pr:`1264`
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

caplog.clear()
search = AutoMLSearch(problem_type='binary', max_iterations=10, max_pipelines=5)
assert "`max_pipelines` will be deprecated in the next release. Use `max_iterations` instead." in caplog.text
assert search.max_iterations == 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

I liked this pattern, we should keep this in mind for future breaking changes!

if max_pipelines:
if not max_iterations:
max_iterations = max_pipelines
logger.warning("`max_pipelines` will be deprecated in the next release. Use `max_iterations` instead.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Food for future thought: I do wonder if we should have used python warnings here instead of / in addition to our logger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree, could have been useful to do it in addition to logging so that it's much more visible to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of doing both? With the way the logger is defined now, logger.warning shows up in stdout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally think the logger is too easy to miss compared to the python warning:
image

image

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 great!

@angela97lin
Copy link
Contributor Author

@dsherry I cleaned up the merge conflict but will need you to merge since codecov is failing 😢

@angela97lin angela97lin added this to the October 2020 milestone Oct 7, 2020
@dsherry
Copy link
Collaborator

dsherry commented Oct 21, 2020

@angela97lin thanks for updating branch, merging shortly

@@ -38,6 +39,12 @@ Release Notes
* Renamed ``LabelLeakageDataCheck`` to ``TargetLeakageDataCheck`` :pr:`1319`


.. warning::
Copy link
Collaborator

@dsherry dsherry Oct 21, 2020

Choose a reason for hiding this comment

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

@angela97lin can you merge this breaking change entry in with the breaking changes section above? I think this got missed during branch update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Updating docs and cleaning up some other doc stuff right now. Will update when this is good to go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsherry
Copy link
Collaborator

dsherry commented Oct 21, 2020

Ok, merging!

@dsherry dsherry merged commit b09f041 into main Oct 21, 2020
1 of 2 checks passed
@angela97lin angela97lin deleted the 1201_max_pipelines_removal branch October 21, 2020 21:52
@dsherry dsherry mentioned this pull request Oct 29, 2020
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.

Remove max_pipelines from AutoMLSearch
3 participants