Improve AUC numeric stability #224
Conversation
SkafteNicki
requested review from
ananyahjha93,
Borda,
justusschock and
tchaton
as code owners
|
Hello @SkafteNicki! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-05-05 17:35:31 UTC |
Codecov Report
@@ Coverage Diff @@
## master #224 +/- ##
=======================================
Coverage 96.80% 96.80%
=======================================
Files 92 92
Lines 3005 3005
=======================================
Hits 2909 2909
Misses 96 96
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| if (dx < 0).any(): | ||
| if (dx <= 0).all(): | ||
| if (dx + tol < 0).any(): | ||
| if (dx <= tol).all(): |
There was a problem hiding this comment.
am I reading it correctly that if I have
dx = [-1e-7, 5e-7, 5e-7, ... 10000 times ...., 5e-7] then we'll deduce incorrect direction here?
There was a problem hiding this comment.
yes, it seems that direction/sign is changed, so shall ve preserve it...
There was a problem hiding this comment.
maybe I am misunderstanding the question...
the tensor
dx = [1e-7, 5e-7, 5e-7, ... 10000 times ...., 5e-7]
implies that direction=1 right? so if we say that numerical instability leads to a change of sign in the first element
dx = [-1e-7, 5e-7, 5e-7, ... 10000 times ...., 5e-7]
then it will still be direction=1 with the change.
There was a problem hiding this comment.
Sorry, let's maybe use this instead:
dx = [-1.0, 5e-7, 5e-7, ... 10000 times ...., 5e-7]
Now both (dx+tol<0).any() and (dx <= tol).all() are true and we'll discover direction as -1, wheras the whole thing is incorrectly sorted.
There was a problem hiding this comment.
thanks, I see the problem now...
Do you have anyway to solve this?
There was a problem hiding this comment.
Numerical issues are usually tricky. I'm suggesting we remove checks for the internal callers of this function, let me quickly show an example.
There was a problem hiding this comment.
@SkafteNicki , check out #230
There was a problem hiding this comment.
@maximsch2 looks good, closing this in favour of yours
Before submitting
What does this PR do?
Fixes #219
In rare case when the input is very large, the
dxherehttps://github.com/PyTorchLightning/metrics/blob/cb6899bb47c7d30f8626d6ef8c28cc29efce82d6/torchmetrics/functional/classification/auc.py#L41-L49
will even for monotone increasing/decreasing (input that is already sorted) result in a mix of positive and negative errors making the algorithm raise the error. This happens when
x[i]andx[i+1]is the same number but due to numerical stabilityx[i+1]-x[i]will randomly either be a small negative or positive number.This PR solves it by adding a small tolerance to the checks.
I checked with the provided repo/script in the discussion that it correctly solves the error.
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 🙃