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 Results Getter #919

Merged
merged 8 commits into from Jul 10, 2020
Merged

Update Results Getter #919

merged 8 commits into from Jul 10, 2020

Conversation

ctduffy
Copy link
Contributor

@ctduffy ctduffy commented Jul 9, 2020

Fixes #274. Makes sure that results are read-only with @ property decorator.

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #919 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #919   +/-   ##
=======================================
  Coverage   99.83%   99.83%           
=======================================
  Files         168      168           
  Lines        8331     8348   +17     
=======================================
+ Hits         8317     8334   +17     
  Misses         14       14           
Impacted Files Coverage Δ
evalml/automl/automl_search.py 98.79% <100.00%> (+0.01%) ⬆️
evalml/tests/automl_tests/test_automl.py 100.00% <100.00%> (ø)
.../automl_tests/test_automl_search_classification.py 100.00% <100.00%> (ø)
...ests/automl_tests/test_automl_search_regression.py 100.00% <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 ea8294e...d840499. Read the comment docs.

evalml/automl/automl_search.py Outdated Show resolved Hide resolved
@ctduffy ctduffy requested review from dsherry and jeremyliweishih Jul 9, 2020
@ctduffy ctduffy added this to the July 2020 milestone Jul 9, 2020
@ctduffy ctduffy marked this pull request as ready for review Jul 9, 2020
@ctduffy ctduffy self-assigned this Jul 9, 2020
@dsherry
Copy link
Collaborator

dsherry commented Jul 9, 2020

@ctduffy I see there's some test failures right now. Please ping me again whenever this is ready for review and I'd be happy to do so. Also please attach this PR to the parent issue #274!

dsherry
dsherry approved these changes Jul 9, 2020
Copy link
Collaborator

@dsherry dsherry left a comment

Left requests for a few minor changes, but after that looks good to me!

docs/source/changelog.rst Outdated Show resolved Hide resolved
evalml/automl/automl_search.py Show resolved Hide resolved
mock_score.return_value = {'Log Loss Binary': 1.0}
automl.search(X, y)

assert automl.results['pipeline_results'][0]['score'] == 1.0
Copy link
Collaborator

@dsherry dsherry Jul 9, 2020

Choose a reason for hiding this comment

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

Nice. Any reason you can't do something like assert automl.results == {'pipeline_results': {[{'score': 1.0, ...}]}, 'search_order': [0]} ?

Copy link
Contributor Author

@ctduffy ctduffy Jul 10, 2020

Choose a reason for hiding this comment

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

Yes. One, it's really long (900 characters). Two, I think that if anyone changes any small part of how results are delivered, they would necessarily have to update the test (if they change this specific part of the dictionary they would also have to update the test, but that seems like it would happen with less frequency). I am happy to add more assertions maybe checking the keys of some dictionaries, but I think checking the whole dict matching isn't necessarily worth it. Let me know what you think.

Copy link
Collaborator

@dsherry dsherry Jul 10, 2020

Choose a reason for hiding this comment

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

That's convincing! Sounds good. I forgot how much stuff ends up in here.

evalml/tests/automl_tests/test_automl.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

LGTM other than Dylan's comments!

automl.results = 2.0

automl.results['pipeline_results'][0]['score'] = 2.0
assert automl.results['pipeline_results'][0]['score'] == 1.0
Copy link
Collaborator

@dsherry dsherry Jul 10, 2020

Choose a reason for hiding this comment

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

Thanks, awesome

@ctduffy ctduffy merged commit 5c282a1 into main Jul 10, 2020
2 checks passed
@ctduffy ctduffy deleted the 274-results branch Jul 10, 2020
@dsherry dsherry mentioned this pull request Jul 16, 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.

Make AutoSearchBase.results a getter instead of accessing internal state
3 participants