Skip to content
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

Merged
merged 7 commits into from
Feb 10, 2024

Conversation

knuesel
Copy link
Member

@knuesel knuesel commented Jan 18, 2024

Fixes #27657.

Currently the check argument of lu 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 PR adds an allowsingular keyword argument to lu and the corresponding issuccess method 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.

The invalid vs rank-defficient factors are distinguished using negative info values in the rank-deficient case, as proposed in #27657.

@mikmoore
Copy link
Contributor

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 try/catch or checking issuccess to detect and handle a problem would no longer be doing that. Bummer.

Very nice to have this as an option, at least.

@ViralBShah ViralBShah added the domain:linear algebra Linear algebra label Jan 22, 2024
stdlib/LinearAlgebra/src/lu.jl Show resolved Hide resolved
stdlib/LinearAlgebra/src/lu.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/lu.jl Outdated Show resolved Hide resolved
@knuesel
Copy link
Member Author

knuesel commented Jan 24, 2024

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).

@ViralBShah
Copy link
Member

Is it ok to merge this PR?

@knuesel
Copy link
Member Author

knuesel commented Feb 2, 2024

I think so (no opposition, and the alternative would be too breaking).

@ViralBShah
Copy link
Member

Would love to get a +1 from @dkarrasch or @jishnub.

@jishnub
Copy link
Contributor

jishnub commented Feb 2, 2024

Looks good to me

knuesel and others added 6 commits February 2, 2024 18:12
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]>
@knuesel
Copy link
Member Author

knuesel commented Feb 2, 2024

Thanks @jishnub . I've just rebased and added a NEWS entry.

Copy link
Member

@dkarrasch dkarrasch left a 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.

NEWS.md Outdated Show resolved Hide resolved
@dkarrasch dkarrasch added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 8, 2024
@vtjnash vtjnash merged commit 3952e78 into JuliaLang:master Feb 10, 2024
5 of 8 checks passed
@oscardssmith oscardssmith removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 10, 2024
@rayegun
Copy link
Member

rayegun commented Feb 11, 2024

We should have a non-throwing equivalent to the singularity checks correct isrankdeficient or something similar? To allow exception free checking of factorization singularity without manually checking the info field etc. issuccess has the new keyword, but I think you'd want to specifically test singularity rather than just relaxing the success criteria.

@knuesel
Copy link
Member Author

knuesel commented Feb 12, 2024

You can do !issuccess(F) && issuccess(F, allowsingular=true). I agree the ergonomics are not great but I'm not sure what would be a good solution. An issingular function has been proposed but met with resistance. Also if we add something like isrankdeficient people will expect it to work for any matrix, but I've seen that frown upon for similar reason (it not being well defined numerically).

@rayegun
Copy link
Member

rayegun commented Feb 12, 2024

Could we add a nothrow kwarg to the check* functions perhaps?

@knuesel
Copy link
Member Author

knuesel commented Feb 12, 2024

I think these are private functions meant to simplify control flow in the factorization code, for example a more accurate name for checknonsingular(info) would be throwifsingular(info)

I'm still not sure a new function is needed: as I understand the info field is meant to tell if the algorithm succeeded. For this purpose issuccess should be good enough. If a user really cares about the rank of U they should probably call rank, which works with a known and configurable tolerance...

Maybe it's best to wait for someone to complain with an actual use case?

maleadt added a commit to JuliaGPU/CUDA.jl that referenced this pull request Apr 9, 2024
maleadt added a commit to JuliaGPU/Metal.jl that referenced this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LinearAlgebra] LU-factorization failure (0.7.0-alpha.0)
8 participants