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 lu decomposition of *diagonal matrices #41288

Closed
wants to merge 2 commits into from
Closed

Add lu decomposition of *diagonal matrices #41288

wants to merge 2 commits into from

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Jun 20, 2021

This adds lu[!] methods for all *diagonal types. As a follow-up to #40831, which made lu work on AbstractMatrix, this reduces the computational load for the structured cases.

Closes #18970. Closes JuliaDiff/TaylorSeries.jl#281

@dkarrasch dkarrasch added the domain:linear algebra Linear algebra label Jun 20, 2021
@dkarrasch
Copy link
Member Author

This also adds rdiv! for Bidiagonal matrices and removes the lu(::StridedMatrix,...) method added in #40831.

@dkarrasch
Copy link
Member Author

@nanosoldier runtests("TaylorSeries", vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run tests: MethodError: no method matching read_pkgs(::String)
Closest candidates are:
  read_pkgs() at /storage/pkgeval/depot/packages/PkgEval/59AzU/src/registry.jl:39
  read_pkgs(!Matched::Vector{String}; registry) at /storage/pkgeval/depot/packages/PkgEval/59AzU/src/registry.jl:39

Logs and partial data can be found here
cc @maleadt

@dkarrasch
Copy link
Member Author

@nanosoldier runtests(["TaylorSeries"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected. A full report can be found here. cc @maleadt

@dkarrasch
Copy link
Member Author

@andreasnoack Would you be able to take a quick look?

While working on this and the other "clean-up" PRs, I was wondering why we have lazy adjoints or transposes of the *diagonal at all. The minute you want to multiply or solve, you materialize the adjoint silently via copy. This concerns only non-Number eltypes. At some point, we must have decided that transposes of Diagonals are never lazy, but the other *diagonal types are also only 2-3 times the memory of Diagonal, so it can hardly be a memory consumption argument.

@dkarrasch
Copy link
Member Author

Gentle bump.

@andreasnoack
Copy link
Member

I've some pending review comments, but first I'd like to ask if these are really useful in the first place? When you call lu you do so because you expect that your matrix doesn't have any structure. To me, lu for structured matrices seems like an unlikely usecase for optimize for. Am I missing something?

I was wondering why we have lazy adjoints or transposes of the *diagonal at all.

I'm pretty sure that this has been discussed at least once but, unfortunately, I don't remember the details. I'd guess that it's to allow for future optimizations that weren't implemented initially because of lack of time.

@dkarrasch
Copy link
Member Author

Am I missing something?

No, you don't. I'm well aware that these by themselves are not very useful. It was two things that triggered this PR: first, the old open issue mentioned in the OP, and second, the copy_* PR which made lu work on AbstractMatrixes (by creating a dense copy) where lu used to fail with a method error before.

@andreasnoack
Copy link
Member

Do you think it is worth the extra lines of code that are required to support optimized versions?

@dkarrasch
Copy link
Member Author

Perhaps not. I'm fine with closing it, except we should remove the StridedMatrix lu method which caused trouble with TaylorSeries.jl because it took over precedence, perhaps together with other small changes in the sequel to #40831 and your review there.

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.

Upcoming lu issue on nightly lufact fails for Diagonal, Bidiagonal, and SymTridiagonal
3 participants