-
-
Notifications
You must be signed in to change notification settings - Fork 24
Add banded paths to generic solve #1372
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
base: master
Are you sure you want to change the base?
Conversation
(This all seems a bit weird to me; I feel like if you want a structured solve algorithm, you should use a structured-matrix type. But I guess since we're already checking for triangular matrices there is some argument for this…) |
I see this as a generic fallback, where the structure may not be obvious from the type, but may be obtained by looking at the values. Suppose we have a fast solver for a |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1372 +/- ##
=======================================
Coverage 93.81% 93.81%
=======================================
Files 34 34
Lines 15717 15723 +6
=======================================
+ Hits 14745 14751 +6
Misses 972 972 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
return Bidiagonal(A, :U) \ B | ||
end | ||
return UpperTriangular(A) \ B | ||
end | ||
if isbanded(A, -1, 1) |
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.
Wouldn't you get this "for free" with istrium1 && istril1
?
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.
Yes, but there were some triangular lu
-related test failures for unusual types, so I've removed the triangular branch for now. We may add this back once those issues are resolved.
Working on #1372, I came across some unusual tests with non-Number `eltype`s, for which this `fill!` fails unless the appropriate zero is used. Probably missing convert methods, but we might as well use the zero of the eltype here.
Currently, the generic solve only checks for diagonal and triangular matrices when dispatching to the faster methods. We also have solve methods for
Bidiagonal
andSymTridiagonal
matrices, so we may add these branches as well.With these,
After this change, almost all the time is spent on checking if the matrix is tridiagonal.