-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
revamp entire metrics #3868
revamp entire metrics #3868
Conversation
Hello @ananyahjha93! Thanks for updating this PR.
Comment last updated at 2020-10-06 19:47:07 UTC |
Codecov Report
@@ Coverage Diff @@
## master #3868 +/- ##
=======================================
- Coverage 88% 83% -4%
=======================================
Files 117 108 -9
Lines 9147 8666 -481
=======================================
- Hits 8007 7215 -792
- Misses 1140 1451 +311 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the new interface. I only had some minor comments (e.g. let's keep functional metrics for now, some naming stuff and some type annotations). I assume docstrings will be added later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we rather move the actual metrics to pl.legacy.metrics then completely before we fill all metrics we had before...
do the same with metric tests aka tests.legacy.metrics
@justusschock we are probably going to deprecate functional metric as with the whole update, reset and compute format, functional metrics don't make sense. If you want a single function to compute your metric value:
Having a functional API with epoch aggregation and ddp sync per step doesn't make sense |
This pull request is now in conflict... :( |
@ananyahjha93 the functional API does none of these things. It's just batch in, Value out and I really would keep these (same reason why pytorch has functionals of their loss functions). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is looking good. Really like many of the changes.
Question: is the idea to do to the hole update in this PR or just lay out the foundation for the metric package here, and the follow up with PRs for individual metrics?
As @justusschock has already mentioned in his review, I see 3 major breaking changes that we at least need to discuss:
- Do we completely remove the functional interface?
- Do we remove support for numpy arrays (by removing
NumpyMetric
subclass)? - If 2 is true, I assume we also remove sklearn interface?
1993031
to
2a09ea2
Compare
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker teddy.koker@gmail.com
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
53cd2af
to
e8cc402
Compare
* removed metric Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * added new metrics Co-authored-by: Teddy Koker teddy.koker@gmail.com * pep8 Co-authored-by: Teddy Koker teddy.koker@gmail.com * pep8 Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * reset in compute, cache compute Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * reduce_ops handling Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * sync -> sync_dist, type annotations Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * wip docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * mean squared error * docstring * added mean ___ error metrics * added mean ___ error metrics * seperated files * accuracy doctest * gpu fix * remove unnecessary mixin * metric and accuracy docstring Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * metric docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * pep8, changelog Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * refactor dist utils, pep8 * refactor dist utils, pep8 Co-authored-by: Teddy Koker <teddy.koker@gmail.com>
* base * add xfail * new test * import * missing import * xfail if not installed include mkpatch fix test * mock comet comet mocks fix test remove dep undo merge duplication * line * line * convert doctest * doctest * docs * prune Results usage in notebooks (#3911) * notebooks * notebooks * revamp entire metrics (#3868) * removed metric Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * added new metrics Co-authored-by: Teddy Koker teddy.koker@gmail.com * pep8 Co-authored-by: Teddy Koker teddy.koker@gmail.com * pep8 Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * win ddp tests skip Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * reset in compute, cache compute Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * reduce_ops handling Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * sync -> sync_dist, type annotations Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * wip docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * mean squared error * docstring * added mean ___ error metrics * added mean ___ error metrics * seperated files * accuracy doctest * gpu fix * remove unnecessary mixin * metric and accuracy docstring Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * metric docs Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * pep8, changelog Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * refactor dist utils, pep8 * refactor dist utils, pep8 Co-authored-by: Teddy Koker <teddy.koker@gmail.com> * Callback docs with autosummary (#3908) * callback docs with autosummary * do not show private methods * callback base docstring * skip some docker builds (temporally pass) (#3913) * skip some docker builds * todos * skip * use badges only with push (#3914) * testtube * mock test tube * mock mlflow * remove mlflow * clean up * test * test * test * test * test * test * code blocks * remove import * codeblock * logger * wandb causes stall Co-authored-by: William Falcon <waf2107@columbia.edu> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Ananya Harsh Jha <ananya@pytorchlightning.ai> Co-authored-by: Teddy Koker <teddy.koker@gmail.com> Co-authored-by: Jeff Yang <ydcjeff@outlook.com>
Hi guys, I just install the pytorch-lightning package with the latest version in master, but found out that the metrics.functional was gone and the metrics were almost empty. Is that true for the moment due to the revamping? Would you recommend a usable version as of now? Thanks |
Co-authored-by: Teddy Koker teddy.koker@gmail.com
What does this PR do?
Deletes metrics
Fixes #3877
Before submitting
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?
Always 💯
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 🙃