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

Binned PR-related metrics #128

Merged
merged 43 commits into from
Apr 13, 2021
Merged

Conversation

maximsch2
Copy link
Contributor

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 #95 .

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 🙃

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #128 (6c3ec24) into master (9bc5b48) will decrease coverage by 16.32%.
The diff coverage is 96.96%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #128       +/-   ##
===========================================
- Coverage   96.03%   79.71%   -16.33%     
===========================================
  Files         176       89       -87     
  Lines        5402     2756     -2646     
===========================================
- Hits         5188     2197     -2991     
- Misses        214      559      +345     
Flag Coverage Δ
Linux 79.71% <96.96%> (+0.41%) ⬆️
Windows 79.71% <96.96%> (+0.41%) ⬆️
cpu 79.71% <96.96%> (-16.29%) ⬇️
gpu ?
macOS 79.71% <96.96%> (-16.29%) ⬇️
pytest 79.71% <96.96%> (-16.33%) ⬇️
python3.6 ?
python3.8 ?
python3.9 ?
torch1.3.1 ?
torch1.4.0 ?
torch1.8.1 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
torchmetrics/__init__.py 100.00% <ø> (ø)
torchmetrics/regression/pearson.py 100.00% <ø> (ø)
...hmetrics/classification/binned_precision_recall.py 96.66% <96.66%> (ø)
torchmetrics/classification/__init__.py 100.00% <100.00%> (ø)
...ics/functional/classification/average_precision.py 100.00% <100.00%> (ø)
torchmetrics/functional/regression/spearman.py 90.69% <100.00%> (-2.33%) ⬇️
torchmetrics/utilities/distributed.py 22.85% <0.00%> (-74.29%) ⬇️
torchmetrics/classification/auc.py 47.61% <0.00%> (-52.39%) ⬇️
torchmetrics/functional/classification/auroc.py 46.15% <0.00%> (-40.01%) ⬇️
torchmetrics/metric.py 55.34% <0.00%> (-39.42%) ⬇️
... and 135 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bc5b48...6c3ec24. Read the comment docs.

@Borda Borda added the enhancement New feature or request label Mar 24, 2021
@Borda Borda marked this pull request as draft March 25, 2021 19:15
@Borda
Copy link
Member

Borda commented Mar 28, 2021

@maximsch2 how is it going here, ready for review?

@maximsch2
Copy link
Contributor Author

@Borda, I need to fix up isort and do a short cleanup, will get to it some time this week.

@maximsch2 maximsch2 changed the title WIP: Binned PR-related metrics Binned PR-related metrics Mar 29, 2021
@maximsch2 maximsch2 marked this pull request as ready for review March 29, 2021 18:05
@Borda
Copy link
Member

Borda commented Mar 29, 2021

@maximsch2 how is the clean-up doing? ;]

@maximsch2
Copy link
Contributor Author

@Borda , I've moved out the function I wanted to do, feel free to take a look now. Another thing I think I should probably do is to move out test data generation code into a central place, but actually curious about your feedback to see if we should just replace the main one with it?

The motivation is to get a reasonably-looking model, so that metrics like recall@precision=0.8 can be computed as the random data never actually reaches that precision :)

@pep8speaks
Copy link

pep8speaks commented Apr 7, 2021

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-13 21:55:49 UTC

@maximsch2
Copy link
Contributor Author

@Borda , mind taking a look now?

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.

it mainly requires docs work:

  • ref to the metric definition
  • argument/options desc
  • examples

tests/classification/inputs.py Outdated Show resolved Hide resolved
tests/classification/test_binned_precision_recall.py Outdated Show resolved Hide resolved
@Borda Borda self-requested a review April 13, 2021 14:28
@maximsch2
Copy link
Contributor Author

Addressed comments. On docs, is your preference to go for strict alphabetical order? I was putting the binned versions close to non-binned ones so that people will notice them together, but I can move the new one to the to the sorted place as well.

@Borda
Copy link
Member

Borda commented Apr 13, 2021

I was putting the binned versions close to non-binned ones so that people will notice them together, but I can move the new one to the to the sorted place as well.

that is also an option, I do not have strong preference here :]

@Borda Borda enabled auto-merge (squash) April 13, 2021 21:56
@Borda Borda added the ready label Apr 13, 2021
@Borda Borda merged commit 41a8848 into Lightning-AI:master Apr 13, 2021
@maximsch2 maximsch2 deleted the binned_metrics branch April 13, 2021 22:18
Borda added a commit to alanhdu/metrics that referenced this pull request Apr 14, 2021
* WIP: Binned PR-related metrics

* attempt to fix types

* switch to linspace to make old pytorch happy

* make flake happy

* clean up

* Add more testing, move test input generation to the approproate place

* bugfixes and more stable and thorough tests

* flake8

* Reuse python zip-based implementation as it can't be reproduced with torch.where/max

* address comments

* isort

* Add docs and doctests, make APIs same as non-binned versions

* pep8

* isort

* doctests likes longer title underlines :O

* use numpy's nan_to_num

* add atol to bleu tests to make them more stable

* atol=1e-2 for bleu

* add more docs

* pep8

* remove nlp test hack

* address comments

* pep8

* abc

* flake8

* remove typecheck

* chlog

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
Co-authored-by: Jirka Borovec <jirka.borovec@seznam.cz>
Borda added a commit that referenced this pull request Apr 14, 2021
* Add AverageMeter

* Fix type annotation to accomodate Python 3.6 bug

* Add tests

* Update changelog

* Add AverageMeter to docs

* fixup! Add AverageMeter to docs

* Code review comments

* Add tests for scalar case

* Fix behavior on PyTorch <1.8

* fixup! Add tests for scalar case

* fixup! fixup! Add tests for scalar case

* Update CHANGELOG.md

* Add Pearson correlation coefficient (#157)

* init files

* rest

* pep8

* changelog

* clamp

* suggestions

* rename

* format

* _sk_pearsonr

* inline

* fix sync

* fix tests

* fix docs

* Apply suggestions from code review

* Update torchmetrics/functional/regression/pearson.py

* atol

* update

* pep8

* pep8

* chlog

* .

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka Borovec <jirka.borovec@seznam.cz>

* Spearman correlation coefficient (#158)

* ranking

* init files

* update

* nearly working

* fix tests

* pep8

* add docs

* fix doctests

* fix docs

* pep8

* isort

* ghlog

* Apply suggestions from code review

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

* Added changes for Test Differentiability [1/n] (#154)

* added test changes

* fix style error

* fixed typo

* added changes for requires_grad

* metrics differentiability testing generalization

* Update tests/classification/test_accuracy.py

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>

* fix tests

* pep8

* changelog

* fix docs

* fix tests

* pep8

* Apply suggestions from code review

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

* Binned PR-related metrics (#128)

* WIP: Binned PR-related metrics

* attempt to fix types

* switch to linspace to make old pytorch happy

* make flake happy

* clean up

* Add more testing, move test input generation to the approproate place

* bugfixes and more stable and thorough tests

* flake8

* Reuse python zip-based implementation as it can't be reproduced with torch.where/max

* address comments

* isort

* Add docs and doctests, make APIs same as non-binned versions

* pep8

* isort

* doctests likes longer title underlines :O

* use numpy's nan_to_num

* add atol to bleu tests to make them more stable

* atol=1e-2 for bleu

* add more docs

* pep8

* remove nlp test hack

* address comments

* pep8

* abc

* flake8

* remove typecheck

* chlog

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
Co-authored-by: Jirka Borovec <jirka.borovec@seznam.cz>

* version + about (#170)

* version + about

* flake8

* try

* .

* fix doc

* overload sig

* fix

* Different import style

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka Borovec <jirka.borovec@seznam.cz>
Co-authored-by: Bhadresh Savani <bhadreshpsavani@gmail.com>
Co-authored-by: Maxim Grechkin <maximsch2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constant-memory implementation of precision-recall related metrics
5 participants