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

Percent better than baseline #1050

Merged
merged 7 commits into from
Aug 18, 2020
Merged

Conversation

freddyaboulton
Copy link
Contributor

Pull Request Description

Closes #984 .

Demo of new rankings table

image


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.

@freddyaboulton freddyaboulton force-pushed the 984-percent-better-than-baseline branch from 669881c to 2d15079 Compare August 13, 2020 16:53
@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1050   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         194      194           
  Lines       10986    11084   +98     
=======================================
+ Hits        10977    11075   +98     
  Misses          9        9           
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.55% <100.00%> (+<0.01%) ⬆️
evalml/objectives/cost_benefit_matrix.py 100.00% <100.00%> (ø)
evalml/objectives/fraud_cost.py 100.00% <100.00%> (ø)
evalml/objectives/lead_scoring.py 100.00% <100.00%> (ø)
evalml/objectives/objective_base.py 100.00% <100.00%> (ø)
evalml/objectives/standard_metrics.py 100.00% <100.00%> (ø)
evalml/tests/automl_tests/test_automl.py 100.00% <100.00%> (ø)
...lml/tests/objective_tests/test_standard_metrics.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 e00fa4b...c791631. Read the comment docs.

@freddyaboulton freddyaboulton marked this pull request as ready for review August 13, 2020 17:07
@freddyaboulton freddyaboulton force-pushed the 984-percent-better-than-baseline branch 3 times, most recently from e29454b to b12fe4e Compare August 14, 2020 14:06
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.

This is great! The tests are solid and the impl is clear and easy to follow.

Approved, pending one request: please update the user guide for objectives to describe the two new required parameters.

evalml/automl/automl_search.py Outdated Show resolved Hide resolved
evalml/automl/automl_search.py Outdated Show resolved Hide resolved
@@ -11,6 +11,8 @@ class CostBenefitMatrix(BinaryClassificationObjective):
name = "Cost Benefit Matrix"
greater_is_better = True
Copy link
Contributor

Choose a reason for hiding this comment

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

@angela97lin I forgot to check this -- we should document why we chose this. I.e. is this score quantifying the cost (lower is better) or the benefit (higher is better)?

For this PR, we just need perfect_score here to align with greater_is_better, which it appears to do

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, I can document this as I work on #1026. It's a little difficult to document this directly on the attribute since we don't expose it but I'll add it to the CostBenefitMatrix class docstring.

evalml/objectives/objective_base.py Outdated Show resolved Hide resolved
evalml/objectives/objective_base.py Outdated Show resolved Hide resolved
evalml/tests/automl_tests/test_automl.py Show resolved Hide resolved
evalml/tests/automl_tests/test_automl.py Show resolved Hide resolved
evalml/tests/objective_tests/test_standard_metrics.py Outdated Show resolved Hide resolved
evalml/tests/objective_tests/test_standard_metrics.py Outdated Show resolved Hide resolved
evalml/objectives/lead_scoring.py Show resolved Hide resolved
@freddyaboulton
Copy link
Contributor Author

@dsherry Thanks for the feedback! I've modified the custom objectives document in the user guide and followed your comments. @kmax12 felt that we should compute the relative difference for all objectives (even if they are percentages) so I've also made that change. That entailed doing some minor tweaks to the tests and removing is_percentage from the objective definitions.

@freddyaboulton freddyaboulton force-pushed the 984-percent-better-than-baseline branch from 988559e to dee80fa Compare August 17, 2020 19:29
@dsherry
Copy link
Contributor

dsherry commented Aug 17, 2020

@freddyaboulton cool, thanks for the heads up. I reread and looks good!

@freddyaboulton freddyaboulton force-pushed the 984-percent-better-than-baseline branch from dee80fa to c791631 Compare August 18, 2020 13:59
@freddyaboulton freddyaboulton merged commit 88a5f1b into main Aug 18, 2020
@freddyaboulton freddyaboulton deleted the 984-percent-better-than-baseline branch August 18, 2020 14:13
@dsherry dsherry mentioned this pull request Aug 25, 2020
@freddyaboulton freddyaboulton restored the 984-percent-better-than-baseline branch February 4, 2021 18:08
@freddyaboulton freddyaboulton deleted the 984-percent-better-than-baseline branch June 17, 2021 21:26
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.

Add percent better than baseline
3 participants