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.

Sign up for GitHub

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

Jump to bottom

Make adjoint and solve work for most factorizations #40899

Merged
merged 15 commits into from
May 30, 2021
Merged

Make adjoint and solve work for most factorizations #40899

merged 15 commits into from
May 30, 2021

Conversation

andreasnoack
Copy link
Member

This should handle the dense cases in JuliaLang/LinearAlgebra.jl#776 for which ldiv! is defined. A few comments

  • For BunchKaufman, I've decided to error out on adjoint for factorizations of complex symmetric matrices. I'm not sure that there is a nice way to apply adjoint to such matrices and I'm pretty sure that the use of it will be minimal.
  • I haven't enabled least squares behavior for Adjoint{LQ} even though it's the same as a QR which has the least square behavior. Lately, there has been some discussion about the least squares behavior of \ being too magical so for now it's an error for Adjoint{LQ}.
  • However, I have enable the minimum norm behavior for Adjoint{<:QRFamily} since it came almost for free.
  • I realized that it was possible make the fallback \ generic and therefore possible to remove definitions for \ that were specific to LQ and QRFamily.
  • Finally, preparing the PR revealed a few bugs that I've fixed.

cc @oxinabox

@andreasnoack andreasnoack requested a review from dkarrasch May 21, 2021 14:27
@oxinabox oxinabox mentioned this pull request May 21, 2021
Open
@andreasnoack andreasnoack force-pushed the an/adjointfactorizations branch from 18fe185 to 998072a Compare May 24, 2021 20:12
@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label May 25, 2021
@vtjnash vtjnash added this to the 1.7 milestone May 25, 2021
@andreasnoack andreasnoack force-pushed the an/adjointfactorizations branch 4 times, most recently from 9e122d5 to 1d6cba1 Compare May 25, 2021 19:47
@andreasnoack andreasnoack mentioned this pull request May 25, 2021
Closed
@andreasnoack
Copy link
Member Author

d87949b fixes JuliaLang/LinearAlgebra.jl#853 so that one should be backported. @KristofferC would you prefer that I move that commit to a separate PR?

@andreasnoack andreasnoack force-pushed the an/adjointfactorizations branch from bfa3dc8 to e94e695 Compare May 27, 2021 11:25
@andreasnoack
Copy link
Member Author

This one is ready

@dkarrasch dkarrasch added the linear algebra Linear algebra label May 28, 2021
andreasnoack added 13 commits May 28, 2021 18:40
@andreasnoack andreasnoack force-pushed the an/adjointfactorizations branch from 7195baa to a6f222c Compare May 28, 2021 16:42
@andreasnoack
Copy link
Member Author

The only failure here is a download failure on Mac due to connectivity issue so I think it's ready.

@andreasnoack andreasnoack merged commit acdffeb into master May 30, 2021
@andreasnoack andreasnoack deleted the an/adjointfactorizations branch May 30, 2021 13:50
@andreasnoack andreasnoack mentioned this pull request Jun 2, 2021
Closed
22 tasks
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
@andreasnoack
Make adjoint and solve work for most factorizations (JuliaLang#40899)
14339aa 
* Add adjoint for Cholesky

* Implement adjoint for BunchKaufman

* Fix ldiv! for adjoints of Hessenbergs

* Add adjoint of LDLt

* Fix return for tall problems in fallback \ method for adjoint of
Factorizations to make \ work for adjoint LQ.

* Fix qr(A)'\b

* Define adjoint for SVD

* Improve promotion in fallback by defining general convert methods
for Factorizations

* Fix ldiv! for SVD

* Restrict the general \ definition that handles over- and underdetermined
systems to LAPACK factorizations

* Remove redundant \ definitions in diagonal.jl

* Add Factorization constructors for SVD

* Disambiguate between the specialized \ for real lhs-complex rhs and
then new \ for LAPACKFactorizations.

* Adjustments based on review

* Fixes for new pivoting syntax
@DilumAluthge DilumAluthge removed the forget me not PRs that one wants to make sure aren't forgotten label Jun 18, 2021
@maleadt
Copy link
Member

maleadt commented Jun 23, 2021

This PR 'broke' CUDA.jl, because it switched from calling similar to zeros (returning an Array).

@andreasnoack suggestions on a fix? Should this code in Base use similar(...) .= 0 (or something like that) to ensure the array type is preserved? Or should CUDA.jl override at a higher level -- we currently implement LinearAlgebra.LAPACK.potrs!, which isn't particularly nice.

@dkarrasch
Copy link
Member

I think something like fill!(similar(A, T, size(...)), zero(T)) should have the same performance in general and do the trick.

@andreasnoack
Copy link
Member Author

andreasnoack commented Jun 23, 2021 •
edited
Loading

I guess we could just change the _zeros definitions. They take the arrays as arguments. @dkarrasch would you be able to prepare a PR?

Maybe we need test LinearAlgebra with a custom array type to ensure that we don't break CUDA. It'll probably be a lot of work so we should fix the immediate issue first.

This was referenced Jun 23, 2021
Closed
Merged
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
@andreasnoack @johanmon
Make adjoint and solve work for most factorizations (JuliaLang#40899)
a89ed9b 
* Add adjoint for Cholesky

* Implement adjoint for BunchKaufman

* Fix ldiv! for adjoints of Hessenbergs

* Add adjoint of LDLt

* Fix return for tall problems in fallback \ method for adjoint of
Factorizations to make \ work for adjoint LQ.

* Fix qr(A)'\b

* Define adjoint for SVD

* Improve promotion in fallback by defining general convert methods
for Factorizations

* Fix ldiv! for SVD

* Restrict the general \ definition that handles over- and underdetermined
systems to LAPACK factorizations

* Remove redundant \ definitions in diagonal.jl

* Add Factorization constructors for SVD

* Disambiguate between the specialized \ for real lhs-complex rhs and
then new \ for LAPACKFactorizations.

* Adjustments based on review

* Fixes for new pivoting syntax
@dlfivefifty
Copy link
Contributor

Assume this makes qr(A)' work? Awesome! Started running into this...

@N5N3 N5N3 mentioned this pull request Dec 12, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxinabox oxinabox oxinabox left review comments

@dkarrasch dkarrasch dkarrasch approved these changes

Assignees

@andreasnoack andreasnoack

Labels
linear algebra Linear algebra
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.
7 participants
@andreasnoack @maleadt @dkarrasch @dlfivefifty @oxinabox @vtjnash @DilumAluthge