-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Plots: Better confusion matrix, and normalized version to #4775
Plots: Better confusion matrix, and normalized version to #4775
Conversation
b9f365e
to
9b355e8
Compare
@elleobrien @dmpetrov Would appreciate your thoughts on this π |
I LOVE this. It's really needed- I had a number of things I disliked about the default template that I was just modifying in my local files, and I suspect these changes will be an upgrade for a lot of data scientists. All good from me- I really don't see any downsides. Switching the scale to normalized 0-1 also has a nice side effect of removing the awkwardly long default legend label "Count of Records", which looks bad in CML reports. 100% approve from me. Thanks @sjawhar! |
Also empty cells is fine because they default to a color that isn't on the color scale- only possible improvement might be to default it to a gray that can be universally understood as Definitely not a blocker. |
This is a greate change @sjawhar!
That is not a blocker. Though I do believe this is kind of bug - scale shows that 0 is light yellow, so I guess it should be marked that way. I think we should ask Vega developers what they think about it. Since their default question channel is stack overflow we could post there. @sjawhar since you are the first to raise this issue, do you want to do that? If not, we will take care of that. |
@pared I agree that it's a bug. I've posted the question on Stack Overflow |
56dd154
to
3a28b78
Compare
@pared @elleobrien Fixed the missing values. It should work in any case where a particular class isn't entirely missing from a column (i.e. class A was never predicted) |
@sjawhar sorry for taking the time with merging. We can merge it right away, will you solve conflicts, or should I do that? |
No worries, I'll take care of it today. |
3a28b78
to
f354bc6
Compare
Rebased on the latest master branch |
This is just amazing, thank you @sjawhar! π π₯ |
Hello! Should we update docs examples per this improvement? Specifically in https://dvc.org/doc/command-reference/plots#example-confusion-matrix and https://dvc.org/doc/command-reference/plots/diff#example-confusion-matrix. |
@jorgeorpinel great point! Created PR. |
Ty @pared |
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. π
Description of Changes
Relates to iterative/dvc-render#20
I really wasn't happy with the default confusion matrix provided in DVC, so I dove into Vega-Lite and figured out how to add a few features:
Screenshots
New
confusion
templateResults of
dvc plots diff
with newconfusion_normalized
templateFeedback Wanted
Here are some stylistic decisions I made that I'm open to reverting in interest of genericness:
nice: true
Also, cells that are zero are empty. This is because of how the grouping works, and I couldn't find a way to "fill in" the missing values. If someone more expert in Vega syntax can help me figure that out, that would be awesome.Fixed!