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 confusion matrix #528

Merged
merged 13 commits into from
Dec 27, 2023
Merged

Add confusion matrix #528

merged 13 commits into from
Dec 27, 2023

Conversation

osanseviero
Copy link
Member

This PR adds the confusion matrix metric. Some flake errors forced me to change two small type checks.

Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@lvwerra
Copy link
Member

lvwerra commented Dec 21, 2023

Actually looks like the doctests are failing. Did you test if the docstring example result match reality?

@osanseviero
Copy link
Member Author

osanseviero commented Dec 23, 2023

It should be good now :)
Edit: test passing locally but not here, will look into it next week

@osanseviero
Copy link
Member Author

Sorry for all the noise with the commits. After some testing, I could pinpoint the issue of different string representations in Windows vs Ubuntu. E.g.

Ubuntu

{'confusion_matrix': array([[1, 0, 1],\n       [0, 2, 0],\n       [1, 1, 0]])}\n

Windows

{'confusion_matrix': array([[1, 0, 1],\n       [0, 2, 0],\n       [1, 1, 0]], dtype=int64)}\n

An alternative would be to just print the shape or sum of the confusion matrix, but this loses a bit of the point. This is also a bit confusing because other metrics such as mean_iou or mse don't face this issue.

@osanseviero
Copy link
Member Author

Ok - the solution was to use ellipsis so it works with or without dtype. Let me know if this is ok. I also changed the Actions workflow so that if the ubuntu test fails, the windows one keeps running. This will consume more resources per workflow run, so let me know if I should revert that part - it would have been useful to identify this bug much faster.

@lvwerra
Copy link
Member

lvwerra commented Dec 27, 2023

Thanks for fixing it. Looks all good!

@lvwerra lvwerra merged commit 8dfe057 into main Dec 27, 2023
8 checks passed
@lvwerra lvwerra deleted the confusion branch December 27, 2023 10:12
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

2 participants