Skip to content

fix an error message of confusion_matrix.IoU#2613

Merged
vfdev-5 merged 3 commits intopytorch:masterfrom
junbao-zhou:fix-confusion-matrix-error-messgae
Jul 16, 2022
Merged

fix an error message of confusion_matrix.IoU#2613
vfdev-5 merged 3 commits intopytorch:masterfrom
junbao-zhou:fix-confusion-matrix-error-messgae

Conversation

@junbao-zhou
Copy link
Copy Markdown
Contributor

@junbao-zhou junbao-zhou commented Jul 13, 2022 •
edited
Loading

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 vfdev-5 requested a review from sadra-barikbin July 14, 2022 11:29
@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Jul 14, 2022

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

sadra-barikbin
sadra-barikbin suggested changes Jul 14, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@sadra-barikbin sadra-barikbin left a comment •
edited
Loading

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
Copy Markdown
Contributor Author

junbao-zhou commented Jul 15, 2022

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
Copy Markdown
Collaborator

vfdev-5 commented Jul 15, 2022 •
edited
Loading

@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
Copy Markdown
Contributor Author

junbao-zhou commented Jul 15, 2022

@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
Copy Markdown
Contributor Author

junbao-zhou commented Jul 16, 2022

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

vfdev-5
vfdev-5 approved these changes Jul 16, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@vfdev-5 vfdev-5 left a comment •
edited
Loading

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

Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

+1 more reviewer

@sadra-barikbin sadra-barikbin sadra-barikbin requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: metrics Metrics module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@junbao-zhou @vfdev-5 @sadra-barikbin