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

Introduce AdjointFactorization not subtyping AbstractMatrix #46874

Merged
merged 24 commits into from
Feb 15, 2023
Merged

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Sep 23, 2022

I have long been bugged by the fact that, on the one hand, you can wrap anything by Adjoint, even types that don't subtype to AbstractMatrix, but once wrapped, they are back in the AbstractMatrix hierarchy. It turns out that this subtyping basically doesn't lead to any usage of fallbacks, except for size(::Adjoint{Factorization}, ::Int)! So, this disentangles that, and even restricts Adjoint/Transpose to wrap only AbstractVecOrMat, which is not necessary. Anyway, it shows that it can be done. Obviously, this is breaking, but I assume it breaks only a few basic packages, but we'll see in a nanosoldier run.

This is a "companion" PR to #46196 and #46233.

@dkarrasch dkarrasch changed the title Introduce AdjointFactorization RFC: Introduce AdjointFactorization Sep 23, 2022
@dkarrasch dkarrasch added kind:breaking This change will break code domain:linear algebra Linear algebra needs pkgeval Tests for all registered packages should be run with this change labels Sep 23, 2022
Comment on lines -63 to 88
# This is a bit odd since the return is not a Factorization but it works well in generic code
Factorization{T}(A::Adjoint{<:Any,<:Factorization}) where {T} =
# This no longer looks odd since the return _is_ a Factorization!
Factorization{T}(A::AdjointFactorization) where {T} =
adjoint(Factorization{T}(parent(A)))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😉

@dkarrasch
Copy link
Member Author

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

@dkarrasch
Copy link
Member Author

I guess if we don't restrict Adjoint to AbsVecOrMat, then this is actually "not" breaking (unless people define functions for Adjoint{<:Any,<:F} where F is a subtype to some factorization from LinearAlgebra). I see usage as in GenericLinearAlgebra for Householder and HouseholderBlock, which both don't subtype to AbstractMatrix, even though their behavior is "matrix-like", e.g., size(H, i) returns 1 if i > 2. If we wanted to restrict Adjoint to AbsVecOrMat, what would we do with Householder[Block]? (1) Introduce a minimal AdjointHouseholder type? (2) Subtype Householder <: AbstractMatrix? Or is that an indication that we shouldn't add the restriction? Do we care that Adoint{Householder} is an AbstractMatrix? @andreasnoack

@dkarrasch
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@dkarrasch
Copy link
Member Author

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@dkarrasch
Copy link
Member Author

I believe the ecosystem is ready for this. Shall we wait until all PRs are merged and run another pkgeval test or can we merge this right away?

@dkarrasch dkarrasch removed the needs pkgeval Tests for all registered packages should be run with this change label Feb 13, 2023
@dkarrasch
Copy link
Member Author

Perhaps this needs a NEWS entry.

@dkarrasch
Copy link
Member Author

@nanosoldier runtests(["SparseMatricesCSR", "RecursiveFactorization", "KLU", "UpdatableQRFactorizations", "SuperLU", "Juniper", "Infernal", "QuantumTomography", "BandedMatrices"])

@nanosoldier
Copy link
Collaborator

Your job failed. Consult the server logs for more details (cc @maleadt).

@maleadt
Copy link
Member

maleadt commented Feb 14, 2023

error during bootstrap:
LoadError("sysimg.jl", 19, LoadError("/source/usr/share/julia/stdlib/v1.10/LinearAlgebra/src/LinearAlgebra.jl", 3, LoadError("/source/usr/share/julia/stdlib/v1.10/LinearAlgebra/src/factorization.jl", 32, LoadError("/source/usr/share/julia/stdlib/v1.10/LinearAlgebra/src/factorization.jl", 32, ErrorException("parsing command `    adjoint(F::Factorization) -> AdjointFactorization\n\nLazy adjoint (conjugate transposition) of the factorization `F`.\n`: special characters \"#{}()[]<>|&*?~;\" must be quoted in commands")))))

@dkarrasch
Copy link
Member Author

@nanosoldier runtests(["SparseMatricesCSR", "RecursiveFactorization", "KLU", "UpdatableQRFactorizations", "SuperLU", "Juniper", "Infernal", "QuantumTomography", "BandedMatrices"])

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@dkarrasch
Copy link
Member Author

The remaining failing packages have open PRs, so I think this is finally ready to go?

@dkarrasch dkarrasch changed the title RFC: Introduce AdjointFactorization Introduce AdjointFactorization not subtyping AbstractMatrix Feb 15, 2023
@dkarrasch
Copy link
Member Author

The only failing test is unrelated. Let's go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:linear algebra Linear algebra kind:breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants