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

fix an error message of confusion_matrix.IoU #2613

Merged

Conversation

junbao-zhou
Copy link
Contributor

@junbao-zhou junbao-zhou commented Jul 13, 2022

Description:

An error message in ignite.metrics.confusion_matrix.IoU is confusing and does not match the condition check, which might lead to misunderstanding. I update the error message to make it match the condition check.

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: metrics Metrics module label Jul 13, 2022
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jul 14, 2022

@BowmanChow thanks for the PR, I let @sadra-barikbin review it and comment out

Copy link
Collaborator

@sadra-barikbin sadra-barikbin left a comment

Choose a reason for hiding this comment

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

Hi @BowmanChow , thanks for the PR. Could you please apply this change to other places of confusion_matrix.py? For example line 396.

@junbao-zhou junbao-zhou force-pushed the fix-confusion-matrix-error-messgae branch from 8e16e75 to 864db9f Compare July 15, 2022 05:01
@junbao-zhou
Copy link
Contributor Author

Hi @sadra-barikbin , I made another change and force-pushed, because I want to keep the commit tree clean. I'm not sure whether force-push is ok. Is it mergable?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jul 15, 2022

@BowmanChow yes, it is ok to push-force. As it is your first contribution we need to approve to run the CI.

By the way, can you please check if there is a confusion matrix test which check for previous message error. If there is one, then it will fail now as you changed the error message.

Please fix:

+ flake8 ignite tests examples --config setup.cfg
ignite/metrics/confusion_matrix.py:242:121: E501 line too long (131 > 120 characters)
ignite/metrics/confusion_matrix.py:396:121: E501 line too long (131 > 120 characters)

@junbao-zhou
Copy link
Contributor Author

@vfdev-5 I'm sorry that I thought my changes were so small and don't need a check. I will have the formatting and test run.

@junbao-zhou junbao-zhou force-pushed the fix-confusion-matrix-error-messgae branch from 864db9f to 31293e1 Compare July 16, 2022 07:39
@junbao-zhou
Copy link
Contributor Author

Hi @vfdev-5 , I've completed the formatting and test_confusion_matrix.py check.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @BowmanChow ! LGTM
CI failures are unrelated. I'll merge your PR once we fixed issues on master and I updated your PR.

@vfdev-5 vfdev-5 enabled auto-merge (squash) July 16, 2022 14:51
@vfdev-5 vfdev-5 merged commit a944081 into pytorch:master Jul 16, 2022
@junbao-zhou junbao-zhou deleted the fix-confusion-matrix-error-messgae branch July 16, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: metrics Metrics module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants