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

Disambiguate structured and abstract matrix multiplication #52464

Merged
merged 9 commits into from
Jan 6, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Dec 9, 2023

The various methods of the form *(::AbstractMatrix, ::Diagonal) for structured matrices lead to method ambiguities in packages that define *(::MyMatrix, ::AbstractMatrix). However, these methods only seem to be defined here to use the first argument to construct the destination, as opposed to the fallback method that uses the second argument. Instead of specializing *, we might as well specialize the call to similar that chooses which matrix to use to construct the destination. This reduces code duplication, as well as avoids these ambiguities altogether. This also makes the destination types of A * B and B * A identical if one of the two is a StructuredMatrix.

After this, for example, both of these produce sparse matrices:

julia> S = sprand(4,4,0.2); B = Bidiagonal(ones(4), ones(3), :U);

julia> S * B
4×4 SparseMatrixCSC{Float64, Int64} with 2 stored entries:
         0.706461  0.706461
                    
                    
                    

julia> B * S
4×4 SparseMatrixCSC{Float64, Int64} with 1 stored entry:
         0.706461    
                    
                    
                    

The former is a Matrix on master.

Close #51354
Address JuliaStats/PDMats.jl#176 on nightly

This PR changes the following behavior:

julia> T = UpperTriangular(Number[i+j for i in 1:4, j in 1:4]);

julia> v = rand(4);

julia> T * v |> eltype
Float64

julia> Array(T) * v |> eltype
Any

to

julia> T * v |> eltype
Any

I'm unsure if the choice of _init_eltype was deliberate here (there is a test that specifically checks that _init_eltype isn't being used for general matrices).

@jishnub jishnub added domain:linear algebra Linear algebra domain:arrays [a, r, r, a, y, s] labels Dec 9, 2023
@jishnub jishnub changed the title Specialize matmul dest for structured matrices Disambiguate structured and abstract matrix multiplication Dec 10, 2023
@dkarrasch
Copy link
Member

That is nice. I believe we have similar * methods in SparseArrays.jl that tell it to call similar on the first argument. We should use this mechanism over there as well.

@dkarrasch
Copy link
Member

I have pushed a commit that removes the clumsy _init* stuff, since unitful arithmetic seems to work just fine with promote_op(matprod, eltype(A), eltype(B)). The inconsistency between triangular and plain matrices was unintended. The promotion mechanism was introduced in a big triangular overhaul, and back then, I didn't extrapolate to the whole LinearAlgebra library.

@dkarrasch
Copy link
Member

Hm, in fact, matprod_dest could be equally well used in the solves, seems like...?

@jishnub
Copy link
Contributor Author

jishnub commented Dec 27, 2023

Looks possible. I'll try to get to this in a couple of days. Thanks for making this simpler!

@dkarrasch
Copy link
Member

BTW, note #35166.

@jishnub
Copy link
Contributor Author

jishnub commented Jan 5, 2024

Is there anything left to be done here?

@dkarrasch
Copy link
Member

I have some local stuff for the determination of the eltype of the destination array, but that could go into another PR.

@dkarrasch dkarrasch added the status:merge me PR is reviewed. Merge when all tests are passing label Jan 6, 2024
@dkarrasch dkarrasch merged commit ea085ea into master Jan 6, 2024
8 checks passed
@dkarrasch dkarrasch deleted the jishnub/matmulambiguity branch January 6, 2024 21:13
@dkarrasch dkarrasch removed the status:merge me PR is reviewed. Merge when all tests are passing label Jan 6, 2024
@singularitti
Copy link
Contributor

Nice work @jishnub @dkarrasch, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent * for AbstractMatrix with AbstractMatrix, and AbstractMatrix with Diagonal, etc.?
3 participants