Skip to content

[Metrics] Fix/4237 auc unstable reorder#4281

Merged
SkafteNicki merged 7 commits intoLightning-AI:masterfrom
ddrevicky:fix/4237-auc-unstable-reorder
Oct 25, 2020
Merged

[Metrics] Fix/4237 auc unstable reorder#4281
SkafteNicki merged 7 commits intoLightning-AI:masterfrom
ddrevicky:fix/4237-auc-unstable-reorder

Conversation

@ddrevicky
Copy link
Copy Markdown
Contributor

@ddrevicky ddrevicky commented Oct 21, 2020 •
edited
Loading

What does this PR do?

Deprecates the reorder parameter to the auc metric because of torch.argsort being unstable.

Fixes #4237

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

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 🙃

@ddrevicky ddrevicky changed the title Fix/4237 auc unstable reorder [Metrics] Fix/4237 auc unstable reorder Oct 21, 2020
@mergify mergify bot requested a review from a team October 21, 2020 11:40
SkafteNicki
SkafteNicki reviewed Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

Please also add note at argument:
https://github.com/PyTorchLightning/pytorch-lightning/blob/d0c96b195f5a22f454c9d73ce595e7911b9dafc1/pytorch_lightning/metrics/functional/classification.py#L808

@mergify mergify bot requested a review from a team October 21, 2020 11:45
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 21, 2020 •
edited
Loading

Codecov Report

Merging #4281 into master will increase coverage by 1%.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master   #4281    +/-   ##
=======================================
+ Coverage      92%     93%    +1%     
=======================================
  Files         111     111            
  Lines        8123    8011   -112     
=======================================
- Hits         7482    7448    -34     
+ Misses        641     563    -78

tchaton
tchaton approved these changes Oct 22, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great catch ! I didn't know argsort was unstable.

@mergify mergify bot requested a review from a team October 22, 2020 07:39
@SkafteNicki
Copy link
Copy Markdown
Collaborator

SkafteNicki commented Oct 22, 2020

@ddrevicky could you please solve the pep8 issue:
./pytorch_lightning/metrics/functional/classification.py:841: [F541] f-string is missing placeholders

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 23, 2020

This pull request is now in conflict... :(

@ddrevicky ddrevicky force-pushed the fix/4237-auc-unstable-reorder branch from 0820cde to a571b78 Compare October 23, 2020 08:44
@ddrevicky ddrevicky force-pushed the fix/4237-auc-unstable-reorder branch from 0894eae to ef04bd4 Compare October 24, 2020 12:19
@SkafteNicki SkafteNicki merged commit 6ad2995 into Lightning-AI:master Oct 25, 2020
@Borda Borda added the bug Something isn't working label Oct 25, 2020
@Borda Borda added the Metrics label Oct 25, 2020
@Borda Borda added this to the 1.0.x milestone Oct 25, 2020
@rohitgr7 rohitgr7 mentioned this pull request Dec 7, 2020
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tchaton tchaton tchaton approved these changes

@justusschock justusschock justusschock approved these changes

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@awaelchli awaelchli Awaiting requested review from awaelchli

@Borda Borda Awaiting requested review from Borda

@nateraw nateraw Awaiting requested review from nateraw

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@teddykoker teddykoker Awaiting requested review from teddykoker

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

+1 more reviewer

@s-rog s-rog s-rog approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

pl:1.0.x

Development

Successfully merging this pull request may close these issues.

[Metrics] AUC reorder is unstable

7 participants

@ddrevicky @SkafteNicki @tchaton @justusschock @s-rog @Borda @edenlightning