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

deprecate _update_called #2141

Merged
merged 14 commits into from
Oct 17, 2023

Conversation

matsumotosan
Copy link
Member

@matsumotosan matsumotosan commented Oct 4, 2023

What does this PR do?

Removes deprecated Metric._update_called in favor of Metric.update_called.

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--2141.org.readthedocs.build/en/2141/

@SkafteNicki
Copy link
Member

@awaelchli
Copy link
Contributor

awaelchli commented Oct 6, 2023

Thank you for taking care of this. With Lightning-AI/pytorch-lightning#18714 Lightning 2.1 will call the public method. Sorry that it was calling the protected API 😅

What is torchmetrics' policy around supporting different lightning versions? Could one use a newer torchmetrics version with an older lightning version? If yes, probably the change here would have to wait otherwise it would break. Anyway, great work on sorting it out for the future! Thanks

@SkafteNicki
Copy link
Member

What is torchmetrics' policy around supporting different lightning versions? Could one use a newer torchmetrics version with an older lightning version? If yes, probably the change here would have to wait otherwise it would break. Anyway, great work on sorting it out for the future! Thanks

@awaelchli I do not think we have an official policy regarding lightning, only torch where we aim for new releases of torchmetrics to be compatible with up to 2 year old releases of torch.
@Borda opinion on what we should do here?

@Borda
Copy link
Member

Borda commented Oct 9, 2023

I do not think we have an official policy regarding lightning

As we are the same group, we try to support as large as we can without too much overhead, but said so, we do not need to be more generous than lightning itself meaning we are actively maintaining 1.9.x and 2.0+ even here we have set range:

pytorch-lightning >=1.6.0, <2.1.0

also addressed in #2129

So, to be clear, this would be a breaking change such that users with TM 1.3+ can't use lighting bellow 2.1, correct?
cc: @lantiga

@Borda Borda added this to the v1.3.0 milestone Oct 9, 2023
@Borda Borda added refactoring refactoring and code health API / design labels Oct 9, 2023
src/torchmetrics/metric.py Outdated Show resolved Hide resolved
@SkafteNicki SkafteNicki marked this pull request as ready for review October 17, 2023 17:00
@Borda Borda enabled auto-merge (squash) October 17, 2023 17:49
@Borda Borda changed the title remove _update_called deprecate _update_called Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #2141 (8b26630) into master (d8a7a55) will increase coverage by 0%.
The diff coverage is 100%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2141   +/-   ##
======================================
  Coverage      87%     87%           
======================================
  Files         293     293           
  Lines       16396   16397    +1     
======================================
+ Hits        14252   14253    +1     
  Misses       2144    2144           

@mergify mergify bot added the ready label Oct 17, 2023
@Borda Borda merged commit 1442e53 into Lightning-AI:master Oct 17, 2023
59 checks passed
@matsumotosan matsumotosan deleted the todo-remove-update-called branch October 18, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants