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

Change import pattern in TM #463

Merged
merged 72 commits into from
Dec 6, 2021
Merged

Change import pattern in TM #463

merged 72 commits into from
Dec 6, 2021

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Aug 18, 2021

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?

What does this PR do?

Fixes #459
Changes the import pattern for metrics requiring third-party packages from

from torchmetrics import FID

to

from torchmetrics.image.fid import FID

the recommended import path will be stated in the example belonging to each metric.

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 🙃

torchmetrics/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ananthsub ananthsub left a comment

Choose a reason for hiding this comment

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

Thanks @SkafteNicki !

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

pls provide justification of the speed-up
this is a breaking change without any back compatibility!

@Borda Borda reopened this Aug 27, 2021
@Borda
Copy link
Member

Borda commented Aug 27, 2021

@SkafteNicki lets follow with #459 (comment)

@SkafteNicki
Copy link
Member Author

@Borda alright lets do it, but we should probably state this somewhere in the documentation?

@mergify mergify bot removed the has conflicts label Aug 28, 2021
torchmetrics/image/__init__.py Show resolved Hide resolved
@Borda
Copy link
Member

Borda commented Dec 1, 2021

@SkafteNicki that is an interesting failer, that shall happen with the original PR not here...
the case is that MAP is asking for some TorchVision version which is not minimal for TM 🐰

@mergify mergify bot added the has conflicts label Dec 3, 2021
@mergify mergify bot removed the has conflicts label Dec 6, 2021
@Borda Borda enabled auto-merge (squash) December 6, 2021 10:52
@mergify mergify bot removed the has conflicts label Dec 6, 2021
@Borda Borda merged commit ba30e63 into master Dec 6, 2021
@Borda Borda deleted the imports branch December 6, 2021 14:31
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 enhancement New feature or request Priority Critical task/issue ready refactoring refactoring and code health
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't import all modules from torchmetrics root
4 participants