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

Add pipeline threshold to confusion matrix returns #3080

Merged
merged 7 commits into from
Nov 22, 2021
Merged

Conversation

bchen1116
Copy link
Contributor

@bchen1116 bchen1116 commented Nov 18, 2021

fix #3079

There are 2 potential issues that I want to raise, although they are not severe enough to block this PR from being merged.
First is the aesthetic of the index when it is very granular, as can be seen here:
image
Note that the threshold is 0.99999..., but looking at the dataframe, it just shows as 1.0, which could be confusing for OS users. By grabbing the index itself,, we can see the actual value. We could stringify the index to have them appear, but this might be annoying for our internal use:
image

The other is when our optimized pipeline threshold doesn't match the best threshold chosen through this method. This can be seen in the problem above, but also can be seen here:
image

Above, the pipeline is optimized using accuracy binary, and we see the pipeline threshold 0.360321 actually has a worse performance value compared to 0.5 with accuracy, which is the ideal value find_confusion_matrix_per_thresholds finds. The differences here are likely how we're finding the optimal threshold, with our optimize_thresholds using gradient descent, and our current method using a simple linear scan. Is this disparity an issue for our users? Discussing with @freddyaboulton, it could be confusing when our optimal thresholds don't match up. However, what would be the best approach to fix this, if necessary?

Again, these two issues shouldn't block the merge of this PR, but are both things I wanted to bring up for discussion.

@bchen1116 bchen1116 self-assigned this Nov 18, 2021
@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #3080 (76b418e) into main (2772536) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3080     +/-   ##
=======================================
+ Coverage   99.8%   99.8%   +0.1%     
=======================================
  Files        313     313             
  Lines      30470   30483     +13     
=======================================
+ Hits       30380   30393     +13     
  Misses        90      90             
Impacted Files Coverage Δ
evalml/model_understanding/decision_boundary.py 100.0% <100.0%> (ø)
...odel_understanding_tests/test_decision_boundary.py 100.0% <100.0%> (ø)

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 2772536...76b418e. Read the comment docs.

Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

Interesting, thanks for outlining these two concerns! Is the first issue because of the way pandas internally handles things? aka it just rounds up rather than printing the actual value? If so, I'd say it's not a big deal given the actual index is still what we expect it to be.

@bchen1116
Copy link
Contributor Author

@angela97lin yep, that was the first concern! Just an aesthetic issue. Filed an issue to address the second here

Copy link
Contributor

@ParthivNaresh ParthivNaresh left a comment

Choose a reason for hiding this comment

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

Looks good man, thanks for filing an issue for the second point. I think we're fine with the aesthetic issue for now.

@bchen1116 bchen1116 merged commit fcfb9dc into main Nov 22, 2021
@chukarsten chukarsten mentioned this pull request Nov 29, 2021
@freddyaboulton freddyaboulton deleted the bc_3079_cf branch May 13, 2022 15:03
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.

Have find_confusion_matrix_per_thresholds return the confusion matrix at the pipeline threshold as well
4 participants