-
-
Notifications
You must be signed in to change notification settings - Fork 26.1k
[MRG] Fixes test_scale_and_stability by clipping small values #13903
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
[MRG] Fixes test_scale_and_stability by clipping small values #13903
Conversation
This test is still failing. |
It's this PR that changed the default behavior of |
I changed the |
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'm not familiar with that algorithm but this seems reasonable as long as CIs are green.
Hmm, I'm surprised to see something like
Is there a bug? Will be grateful if you can briefly explain what's happening here. |
I quickly went through the code, it's strange that scaling the same data for multiple times will introduce such a big difference? |
This is likely a 32-bit numerical issue, since the tests pass for the other environments. |
When I set the tolerance too low, it went below |
Heading to bed and thanks for your hard work here :) |
Quick recap of our pair debugging sesh with @thomasjpfan:
There are 2 solutions:
|
@NicolasHug @thomasjpfan tests are still failing |
This error stems from how
The PR fixes this issue by replacing the small values with zero. |
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.
So changing cond
in the call to pinv2
doesn't work?
I'm fine with this anyway, I don't think there's much more we can do.
@qinhanmin2014 you might want to have another look since this has changed since you approved it.
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.
This requires a what's new (0.21.2) entry.
Do the original tests now pass with the small value clipping?
Does the eigenvalue conditioning issue relate to #12145?
I think we would need two conditions, one for single precision and another for double precision. (The double precision would need to be scaled up more). This was how it was done in
This is related. The
Yes. The value clipping forces the eigenvalue to zero, thus forcing |
If the original tests pass, why have we modified them? |
Thanks for the fix, @thomasjpfan |
Reference Issues/PRs
Fixes #6279
What does this implement/fix? Explain your changes.
Clips small values in
_PLS.fit
.This has been causing errors in our nightly tests (
scipy-dev
). This will start to cause errors in our regular tests since scipy 1.13.0 came out.