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

Cleaned up some tests in test_loss.py #2529

Merged
merged 9 commits into from
Apr 6, 2022
Merged

Cleaned up some tests in test_loss.py #2529

merged 9 commits into from
Apr 6, 2022

Conversation

asmayer
Copy link
Contributor

@asmayer asmayer commented Mar 25, 2022

Addresses #2522

Description:

Cleans up some tests in test_loss.py

Something worth noting:
test_compute_on_criterion and test_compute were identical. I addressed this by having test_compute_on_criterion call test_compute but perhaps this test should be removed entirely

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)

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 @asmayer
Please, run code formatting tools to make sure if the code is properly formatted: https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md#formatting-code

Concerning your note about test_compute_on_criterion and test_compute were identical.
Actually, it is not completely true.
test_compute is using nll_loss which is functional NLL implementation and it is a function. test_compute_on_criterion is using nn.NLLLoss() which nn.Module.

tests/ignite/metrics/test_loss.py Outdated Show resolved Hide resolved
@asmayer
Copy link
Contributor Author

asmayer commented Mar 25, 2022

Hey @vfdev-5,

Thanks for the review. I committed some changes that address the two issues you pointed out. Both of which could have have been easy avoided by reading the contributing guidelines along with the source code more carefully. I am a bit new to contributing to open source but this was simply careless on my end. I apologize for all of that.

@asmayer asmayer changed the title cleaned up some tests in test_loss.py Cleaned up some tests in test_loss.py Mar 25, 2022
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 updates @asmayer
I left few more comments to adress

tests/ignite/metrics/test_loss.py Outdated Show resolved Hide resolved
tests/ignite/metrics/test_loss.py Outdated Show resolved Hide resolved
tests/ignite/metrics/test_loss.py Outdated Show resolved Hide resolved
tests/ignite/metrics/test_loss.py Outdated Show resolved Hide resolved
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 @asmayer and sorry for such delay 🙏

@vfdev-5 vfdev-5 merged commit a359694 into pytorch:master Apr 6, 2022
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