-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
LU factorization: add allowsingular keyword argument #52957
Conversation
It seems more Julian to always allow singular and to only to throw if one attempts to use it in an inverse context. But it looks like it would be breaking to make that default, because people who previously depended on a Very nice to have this as an option, at least. |
Thanks for the review @dkarrasch I agree with @mikmoore that it would be better to always allow singular and let the code produce an error later if it's wrongly used, but thought that would be too breaking (I'm happy to make another PR with that behavior if there's agreement that it would be an acceptable change). |
Is it ok to merge this PR? |
I think so (no opposition, and the alternative would be too breaking). |
Would love to get a +1 from @dkarrasch or @jishnub. |
Looks good to me |
Currently the `check` argument conflates failed factorization (invalid factors) and valid factorizations that have a rank-deficient factor, and the `show` value for the factorization is misleading in the latter case. This adds an `allowsingular` keyword argument to `lu` and the corresponding `issuccess` methods to treat valid but rank-deficient factors as success. It also changes `show` to show the factors as usual in such cases, but with a "rank-deficient" note. Fix JuliaLang#27657
Reword compat mention Co-authored-by: Daniel Karrasch <[email protected]>
Reword compat mention Co-authored-by: Daniel Karrasch <[email protected]>
Co-authored-by: Jishnu Bhattacharya <[email protected]>
639df0d
to
07a92e0
Compare
Thanks @jishnub . I've just rebased and added a NEWS entry. |
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.
Up to the typo, LGTM.
We should have a non-throwing equivalent to the singularity checks correct |
You can do |
Could we add a nothrow kwarg to the |
I think these are private functions meant to simplify control flow in the factorization code, for example a more accurate name for I'm still not sure a new function is needed: as I understand the Maybe it's best to wait for someone to complain with an actual use case? |
Fixes #27657.
Currently the
check
argument oflu
conflates failed factorization (invalid factors) and valid factorizations that have a rank-deficient factor, and theshow
value for the factorization is misleading in the latter case.This PR adds an
allowsingular
keyword argument tolu
and the correspondingissuccess
method to treat valid but rank-deficient factors as success. It also changesshow
to show the factors as usual in such cases, but with a "rank-deficient" note.The invalid vs rank-defficient factors are distinguished using negative
info
values in the rank-deficient case, as proposed in #27657.