-
Notifications
You must be signed in to change notification settings - Fork 412
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
Remove numpy dependency from the core library #212
Conversation
Hello @maximsch2! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-04-30 12:16:53 UTC |
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
==========================================
- Coverage 96.66% 96.65% -0.01%
==========================================
Files 180 180
Lines 5750 5744 -6
==========================================
- Hits 5558 5552 -6
Misses 192 192
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
agree with @Borda on this one, no need to remove |
I don't think pytorch requires numpy to run, it's likely for tests as well. In fact, we are running pytorch in builds without numpy so it's for sure not required. In this particular case, the core library doesn't really require numpy, so I'm thinking we should keep it lean especially given it's a no-cost thing for us. The two current uses for numpy in torchmetrics were:
Both are not needed for metrics to function. |
@SkafteNicki @Borda one of the concerns is around backwards compatibility for metrics. numpy introduces a large surface area which can make this harder to manage. given how minimal the usage is in torchmetrics now, we'd prefer to keep the library lean and use torch primitives instead to better control for this |
Ok, I am fine with dropping it 🐰 |
@maximsch2 and @ananthsub great points, also fine with dropping it, did not really have a strong opinion on this :] |
Before submitting
What does this PR do?
Remove numpy dependency that seemed to have been added mostly for one test to make it cleaner. Instead moved the function that needs to support both Tensor and ndarray to test utils and removed the dependency from the core library.
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 🙃