-
-
Notifications
You must be signed in to change notification settings - Fork 55
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 needs_square_A
trait
#400
Conversation
Codecov Report
@@ Coverage Diff @@
## main #400 +/- ##
==========================================
- Coverage 68.95% 68.37% -0.58%
==========================================
Files 26 26
Lines 1907 1929 +22
==========================================
+ Hits 1315 1319 +4
- Misses 592 610 +18
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
d814042
to
0cf30e1
Compare
0cf30e1
to
988e3f7
Compare
src/LinearSolve.jl
Outdated
function for custom algorithms! | ||
""" | ||
needs_square_A(::Nothing) = false # Linear Solve automatically will use a correct alg! | ||
function needs_square_A(alg::SciMLLinearSolveAlgorithm) |
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.
Just remove the fallback please.
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.
Or rather, the fallback is true. That's a safe fallback, methods can opt-in to supporting non-square, but anything that supports non-square supports square so that's a safe assumption.
DefaultAlgorithmChoice.QRFactorization | ||
if is_underdetermined(A) | ||
# Underdetermined | ||
DefaultAlgorithmChoice.QRFactorizationPivoted |
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.
QRFactorization is already pivoted?
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.
oh I see. @YingboMa what do you think of just always defaulting to pivoted QR?
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.
Yeah, pivoted QR handles degenerate systems too.
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.
But it's quite a bit slower though.
23297f9
to
45adbd0
Compare
Makes the following changes:
needs_square_A
trait for solvers