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

retain type of _modules when renaming keys #2793

Merged
merged 19 commits into from
Oct 22, 2024

Conversation

bfolie
Copy link
Contributor

@bfolie bfolie commented Oct 21, 2024

What does this PR do?

Fixes #2789

Before submitting
  • Was this discussed/agreed 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 🙃


📚 Documentation preview 📚: https://torchmetrics--2793.org.readthedocs.build/en/2793/

@Borda Borda requested a review from stancld as a code owner October 21, 2024 17:56
@Borda Borda enabled auto-merge (squash) October 21, 2024 18:00
@Borda Borda added the bug / fix Something isn't working label Oct 21, 2024
@Borda Borda disabled auto-merge October 21, 2024 18:17
@bfolie
Copy link
Contributor Author

bfolie commented Oct 21, 2024

Thanks @Borda, sorry for the confusion about the typing -- I haven't used flake8-comprehsions before

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68%. Comparing base (d1d3242) to head (35ac437).
Report is 2 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2793   +/-   ##
======================================
  Coverage      68%     68%           
======================================
  Files         322     322           
  Lines       18046   18047    +1     
======================================
+ Hits        12326   12327    +1     
  Misses       5720    5720           

@Borda
Copy link
Member

Borda commented Oct 21, 2024

@SkafteNicki one issue remained:

FAILED unittests/wrappers/test_multitask.py::test_error_on_wrong_keys - AssertionError: Regex pattern did not match.
 Regex: "Expected\\ arguments\\ `task_preds`\\ and\\ `task_targets`\\ to\\ have\\ the\\ same\\ keys\\ as\\ the\\ wrapped\\ `task_metrics`\\.\\ Found\\ task_preds\\.keys\\(\\)\\ =\\ dict_keys\\(\\['Classification'\\]\\),\\ task_targets\\.keys\\(\\)\\ =\\ dict_keys\\(\\['Classification',\\ 'Regression'\\]\\)\\ and\\ self\\.task_metrics\\.keys\\(\\)\\ =\\ odict_keys\\(\\['Classification',\\ 'Regression'\\]\\)"
 Input: "Expected arguments `task_preds` and `task_targets` to have the same keys as the wrapped `task_metrics`. Found task_preds.keys() = dict_keys(['Classification']), task_targets.keys() = dict_keys(['Classification', 'Regression']) and self.task_metrics.keys() = dict_keys(['Classification', 'Regression'])"

@Borda Borda changed the title retain type of _modules when renaming keys retain type of _modules when renaming keys Oct 22, 2024
@Borda Borda enabled auto-merge (squash) October 22, 2024 08:42
@Borda Borda disabled auto-merge October 22, 2024 09:42
@Borda Borda merged commit be080dd into Lightning-AI:master Oct 22, 2024
28 of 35 checks passed
@mergify mergify bot added the ready label Oct 22, 2024
Borda pushed a commit that referenced this pull request Oct 22, 2024
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka B <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nicki Skafte <[email protected]>
(cherry picked from commit be080dd)
Borda pushed a commit that referenced this pull request Oct 22, 2024
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka B <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nicki Skafte <[email protected]>
(cherry picked from commit be080dd)
Borda pushed a commit that referenced this pull request Oct 22, 2024
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka B <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nicki Skafte <[email protected]>
(cherry picked from commit be080dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing _modules dict type in Pytorch 2.5 leads to failing collections test
3 participants