-
Notifications
You must be signed in to change notification settings - Fork 87
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
Change colors of confusion matrix to shades of blue and change the axis order to match scikit-learn's #426
Conversation
Codecov Report
@@ Coverage Diff @@
## master #426 +/- ##
=======================================
Coverage 97.47% 97.47%
=======================================
Files 105 105
Lines 3287 3290 +3
=======================================
+ Hits 3204 3207 +3
Misses 83 83 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good. one other change would be to change the axis order to match scikit learn as well. so the correct predicts are the diagonal that go top left to bottom right.
@kmax12 done! I updated the image I attached with the new axis order :) |
@kmax12 okay, I think I actually updated the plot to match sklearn's now (see attached photo) 👍 |
@@ -206,14 +206,19 @@ def generate_confusion_matrix(self, pipeline_id, fold_num=None): | |||
conf_mat = data[fold_num] | |||
labels = conf_mat.columns | |||
|
|||
# reverse columns in confusion matrix to change axis order to match sklearn's | |||
conf_mat = conf_mat.iloc[:, ::-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to do this here or in get_confusion_matrix_data
function? ideally that function returns the data as closely as possible to what makes it easy to create the graph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I agree that given what we previously talked about (generate_confusion_matrix
should simply take in the data and graph it) that makes sense. I've updated the PR to move the reversal to get_confusion_matrix_data
though I wonder if the confusion matrix data feels less intuitive to read now 🤔 What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #425.