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

Removed input_order from text unit tests #717

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

jscottcronin
Copy link
Contributor

What does this PR do?

Historical Work:

The following issue was created to align input order for preds and targets in the text (NLG) module: #686
The following PR was merged with the fix: #696

Fixes #704
The work in this PR removes references to input_order in the torchmetrics.tests.text.helpers._class_test and torchmetrics.tests.text.helpers._functional_test. All unit tests also remove this parameter from calls to these methods. All unit tests are passing locally.

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?


Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #717 (0480dfc) into master (4c2106b) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #717   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         166    166           
  Lines        6424   6424           
=====================================
  Hits         6117   6117           
  Misses        307    307           

@Borda Borda added enhancement New feature or request test / CI testing or CI labels Jan 5, 2022
@Borda Borda enabled auto-merge (squash) January 5, 2022 20:44
@jscottcronin
Copy link
Contributor Author

@Borda - Any thoughts on why codecov shows a loss of 22% coverage when I'm fully up to date with master? Also, do we know why some of the test environments fail in CI/CD? Here is an example:
image

@Borda
Copy link
Member

Borda commented Jan 5, 2022

@Borda - Any thoughts on why codecov shows a loss of 22% coverage when I'm fully up to date with master?

you just need to wait until all tests are finished, mainly then GPU tests

Also, do we know why some of the test environments fail in CI/CD? Here is an example: image

there is something wrong with the package we depend on, not sure what... @quancs mind have know?

@mergify mergify bot added the ready label Jan 5, 2022
Copy link
Contributor

@stancld stancld left a comment

Choose a reason for hiding this comment

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

LGTM!

@Borda Borda disabled auto-merge January 5, 2022 22:49
@Borda Borda merged commit 89f603b into Lightning-AI:master Jan 5, 2022
@quancs
Copy link
Member

quancs commented Jan 6, 2022

there is something wrong with the package we depend on, not sure what... @quancs mind have know?

Seems something wrong with the numpy version.

@Borda
Copy link
Member

Borda commented Jan 6, 2022

there is something wrong with the package we depend on, not sure what... @quancs mind have know?

Seems something wrong with the numpy version.

we can try to reinstall or do you have some specific version we shall use?

@quancs
Copy link
Member

quancs commented Jan 6, 2022

@Borda I checked the differences of installed packages in the test environment. Below are the differences. I will open another PR to verify which is the problem. ^^
图片

@quancs quancs mentioned this pull request Jan 6, 2022
4 tasks
@jscottcronin jscottcronin deleted the tests/704 branch January 6, 2022 20:59
Borda pushed a commit that referenced this pull request Jan 10, 2022
@Borda Borda added this to the v0.8 milestone May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready test / CI testing or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean text tests - remove input_order argument
4 participants