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

allow add_to_rankings to work before search #1250

Merged
merged 6 commits into from Oct 1, 2020
Merged

Conversation

bchen1116
Copy link
Contributor

fix #900

Implementation to allow add_to_rankings() to be called before AutoMLSearch().search().

@bchen1116 bchen1116 self-assigned this Sep 30, 2020
@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #1250 into main will increase coverage by 8.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1250      +/-   ##
==========================================
+ Coverage   91.91%   99.93%   +8.01%     
==========================================
  Files         207      207              
  Lines       12898    12927      +29     
==========================================
+ Hits        11855    12918    +1063     
+ Misses       1043        9    -1034     
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.58% <100.00%> (+0.41%) ⬆️
evalml/tests/automl_tests/test_automl.py 100.00% <100.00%> (ø)
...s/prediction_explanations_tests/test_algorithms.py 100.00% <0.00%> (+1.11%) ⬆️
evalml/tests/component_tests/test_components.py 100.00% <0.00%> (+1.16%) ⬆️
evalml/tests/component_tests/test_utils.py 100.00% <0.00%> (+1.85%) ⬆️
evalml/tests/pipeline_tests/test_pipelines.py 100.00% <0.00%> (+3.45%) ⬆️
...derstanding/prediction_explanations/_algorithms.py 97.14% <0.00%> (+4.28%) ⬆️
evalml/utils/gen_utils.py 99.11% <0.00%> (+5.30%) ⬆️
evalml/pipelines/pipeline_base.py 100.00% <0.00%> (+6.14%) ⬆️
... and 18 more

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 8cdfe14...cbf65f2. Read the comment docs.

@bchen1116 bchen1116 marked this pull request as ready for review September 30, 2020 22:23
dsherry
dsherry approved these changes Oct 1, 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.

@bchen1116 awesome! I left one test comment but once that's outta the way, 🚢



@patch('evalml.pipelines.RegressionPipeline.score')
@patch('evalml.pipelines.RegressionPipeline.fit')
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to mock fit if we're not calling automl.search

Do we have separate coverage already for calling add_to_rankings after search has been called? It would be nice to add that to this test since you're already mocking fit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are tests that call search after add_to_rankings. I'll remove the fit mocks

Copy link
Contributor

Choose a reason for hiding this comment

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

Would also be nice to make sure automl.search behaves properly if add_to_rankings is called first as well.

@@ -798,8 +805,7 @@ def add_to_rankings(self, pipeline, X, y):
if not isinstance(y, pd.Series):
y = pd.Series(y)

if not self.has_searched:
raise RuntimeError("Please run automl search before calling `add_to_rankings()`")
self._set_data_split(X)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks great!

@bchen1116 bchen1116 merged commit e81b19d into main Oct 1, 2020
@dsherry dsherry mentioned this pull request Oct 29, 2020
@freddyaboulton freddyaboulton deleted the bc_900_rankings branch May 13, 2022 14:51
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.

Allow add_to_rankings() to be called before search is ran
3 participants