Skip to content

Conversation

@ddrevicky
Copy link
Contributor

@ddrevicky ddrevicky commented Oct 21, 2020

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
Copy link
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.

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

codecov bot commented Oct 21, 2020

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     

Copy link
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
Collaborator

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

@mergify
Copy link
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
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Metrics] AUC reorder is unstable

7 participants