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

Modify calculate_percent_difference so that it can handle negative values #1100

Merged
merged 4 commits into from Aug 25, 2020

Conversation

freddyaboulton
Copy link
Contributor

@freddyaboulton freddyaboulton commented Aug 25, 2020

Pull Request Description

Let's say for CostBenefitMatrix, the model score is 5 and the baseline score is -10.

Right now, we compute 100 * (5 - (-10))/ -10 = -150%, indicating a decrease in performance when the right answer should be a 150% increase in performance.

This PR fixes that and adds coverage.


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.

@codecov
Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #1100 into main will increase coverage by 5.84%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1100      +/-   ##
==========================================
+ Coverage   94.06%   99.91%   +5.84%     
==========================================
  Files         192      192              
  Lines       10736    10751      +15     
==========================================
+ Hits        10099    10742     +643     
+ Misses        637        9     -628     
Impacted Files Coverage Δ
evalml/objectives/objective_base.py 100.00% <100.00%> (ø)
...lml/tests/objective_tests/test_standard_metrics.py 100.00% <100.00%> (ø)
evalml/tests/component_tests/test_components.py 100.00% <0.00%> (+0.19%) ⬆️
evalml/automl/automl_search.py 99.56% <0.00%> (+0.43%) ⬆️
...s/prediction_explanations_tests/test_algorithms.py 100.00% <0.00%> (+1.11%) ⬆️
evalml/tests/pipeline_tests/test_pipelines.py 100.00% <0.00%> (+1.38%) ⬆️
evalml/tests/component_tests/test_utils.py 100.00% <0.00%> (+3.57%) ⬆️
...derstanding/prediction_explanations/_algorithms.py 100.00% <0.00%> (+4.28%) ⬆️
evalml/pipelines/pipeline_base.py 100.00% <0.00%> (+5.77%) ⬆️
evalml/tests/utils_tests/test_dependencies.py 100.00% <0.00%> (+6.25%) ⬆️
... and 15 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 b7442cf...8efcfaa. Read the comment docs.

@freddyaboulton freddyaboulton marked this pull request as ready for review Aug 25, 2020
elif baseline_score < score and cls.greater_is_better:
increase = True
elif baseline_score > score and cls.greater_is_better:
increase = False
elif baseline_score < score and not cls.greater_is_better:
increase = False
else:
increase = True
Copy link
Contributor

@angela97lin angela97lin Aug 25, 2020

Choose a reason for hiding this comment

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

Hmm, I wonder if this can be simplified a little? Suggestion:

increase = True
if (baseline_score > score and cls.greater_is_better) or (baseline_score < score and not cls.greater_is_better):
    increase = False

Copy link
Contributor Author

@freddyaboulton freddyaboulton Aug 25, 2020

Choose a reason for hiding this comment

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

Good call! Really makes things cleaner. I think I was just being paranoid I would miss a case hehe

Copy link
Contributor

@angela97lin angela97lin Aug 25, 2020

Choose a reason for hiding this comment

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

Hahaha I don't blame you, I had to write it all out and make sure too 😂

Copy link
Contributor

@angela97lin angela97lin left a comment

Test case looks good, just commented on maybe simplifying the checks!

@freddyaboulton freddyaboulton force-pushed the percent-better-baseline-negative-values branch from 0e6d9f0 to 0850cf9 Compare Aug 25, 2020
@freddyaboulton freddyaboulton force-pushed the percent-better-baseline-negative-values branch from 0850cf9 to f9dba67 Compare Aug 25, 2020
Copy link
Collaborator

@dsherry dsherry left a comment

Lgtm!


increase = True
if (baseline_score > score and cls.greater_is_better) or (baseline_score < score and not cls.greater_is_better):
increase = False
Copy link
Collaborator

@dsherry dsherry Aug 25, 2020

Choose a reason for hiding this comment

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

Cool. My only thought is maybe you can invert it so you don't need the not in not increase

Copy link
Contributor Author

@freddyaboulton freddyaboulton Aug 25, 2020

Choose a reason for hiding this comment

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

Done!

@freddyaboulton freddyaboulton merged commit e12671f into main Aug 25, 2020
@dsherry dsherry mentioned this pull request Aug 25, 2020
@freddyaboulton freddyaboulton deleted the percent-better-baseline-negative-values branch Oct 22, 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.

None yet

4 participants