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

Add RowNonZero pivoting strategy to lu #44571

Merged
merged 6 commits into from
May 13, 2022
Merged

Add RowNonZero pivoting strategy to lu #44571

merged 6 commits into from
May 13, 2022

Conversation

dkarrasch
Copy link
Member

This implements another pivoting strategy, and allows to specify a pivoting strategy other than the default RowMaximum depending on the eltype.

Comments welcome regarding the naming.

This closes JuliaLang/LinearAlgebra.jl#431 and closes #22232.

@dkarrasch dkarrasch added the linear algebra Linear algebra label Mar 11, 2022
@dkarrasch
Copy link
Member Author

@ChrisRackauckas Can you help me ping potentially interested people, please? I assume people from the symbolic computation community. Or is that handled elsewhere anyway?

@ChrisRackauckas
Copy link
Member

@YingboMa @shashi

@YingboMa
Copy link
Contributor

We don't really do this in Symbolics. We pivot by the least complicated term.

@dkarrasch
Copy link
Member Author

I believe this is in principle uncontroversial, but perhaps we should bikeshed on the name of the pivot strategy? And perhaps we should announce it in NEWS?

@ViralBShah ViralBShah added the needs news A NEWS entry is required for this change label Mar 24, 2022
@ViralBShah
Copy link
Member

What do other software packages use as a name for this pivoting strategy? Do we have it in LAPACK or SuiteSparse?

@ViralBShah
Copy link
Member

I don't have a view on the name. Perhaps worth checking with @stevengj and @andreasnoack.

Seems like it should be mentioned in the documentation. I'm adding the needs docs label (but it is possible I missed it in the PR).

@ViralBShah ViralBShah added the needs docs Documentation for this change is required label Mar 28, 2022
@stevengj
Copy link
Member

stevengj commented Apr 9, 2022

You would never use this as a pivoting strategy with floating-point values, except as a teaching tool (it matches the typical strategy for hand computations), because it is numerically unstable — so you won't see this in libraries like LAPACK, SuiteSparse, numpy etc.

But it makes sense to have it available for more generic number-like types (as well as for pedagogy). The name seems fine to me.

@dkarrasch
Copy link
Member Author

I'd like to request a review of 579ba89, which adds docs and NEWS. Once this is appropriate and polished, this PR can be finally merged.

@dkarrasch dkarrasch removed needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels May 13, 2022
NEWS.md Outdated Show resolved Hide resolved
@dkarrasch
Copy link
Member Author

Thank you very much @stevengj!

@dkarrasch dkarrasch added the merge me PR is reviewed. Merge when all tests are passing label May 13, 2022
@dkarrasch dkarrasch changed the title RFC: Add RowNonZero pivoting strategy to lu Add RowNonZero pivoting strategy to lu May 13, 2022
@dkarrasch dkarrasch merged commit f9aa28f into master May 13, 2022
@dkarrasch dkarrasch deleted the dk/lupivot branch May 13, 2022 18:51
@dkarrasch dkarrasch removed the merge me PR is reviewed. Merge when all tests are passing label May 13, 2022
@j-fu
Copy link

j-fu commented Oct 26, 2022

See https://discourse.julialang.org/t/from-toy-to-production-code-with-sparse-matrix-and-sparse-direct-factorizations/89105/13 + follow-ups for potential interests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generic lufact requires more than it should
6 participants