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 *(::Diagonal, ::Diagonal, ::Diagonal) (#49005) #49007

Merged
merged 6 commits into from
Mar 16, 2023

Conversation

dlfivefifty
Copy link
Contributor

This adds a special case for triple multiplication of diagonals to ensure that the result is a Diagonal, see JuliaLang/LinearAlgebra.jl#992.

@dlfivefifty dlfivefifty added this to the 1.9 milestone Mar 15, 2023
@dlfivefifty dlfivefifty requested a review from jishnub March 15, 2023 10:31
@jishnub
Copy link
Contributor

jishnub commented Mar 15, 2023

Currently, this uses broadcasting, and I wonder if it might not be better to make the result of the broadcast operation return a Diagonal instead of special-casing this 3-term multiplication?

julia> (1:4) .* Diagonal(1:4) .* (1:4)' # may return a Diagonal
4×4 Matrix{Int64}:
 1  0   0   0
 0  8   0   0
 0  0  27   0
 0  0   0  64

In fact, each component may be evaluated to a Diagonal here:

julia> (1:4) .* Diagonal(1:4)
4×4 Matrix{Int64}:
 1  0  0   0
 0  4  0   0
 0  0  9   0
 0  0  0  16

julia> Diagonal(1:4) .* (1:4)'
4×4 Matrix{Int64}:
 1  0  0   0
 0  4  0   0
 0  0  9   0
 0  0  0  16

@dlfivefifty
Copy link
Contributor Author

That would require special case-ing broadcast for * which would be a bigger project. I'm just trying to fix the regression introduced by a special case *(::Diagonal, ::AbstractMatrix, ::Diagonal) added.

@jishnub
Copy link
Contributor

jishnub commented Mar 15, 2023

This is tricky with floating-point values, as NaN and Inf don't preserve the Diagonal nature:

julia> Diagonal([1,NaN]) * Diagonal(1:2) * Diagonal([1,NaN])
2×2 Matrix{Float64}:
   1.0  NaN
 NaN    NaN

@KristofferC
Copy link
Member

This is tricky with floating-point values, as NaN and Inf don't preserve the Diagonal nature:

This is already the case with e.g. sparse arrays. And it was like that on 1.8 as well.

@martinholters
Copy link
Member

How does * with even more Diagonal arguments fare?

@jishnub
Copy link
Contributor

jishnub commented Mar 15, 2023

How does * with even more Diagonal arguments fare?

julia> Diagonal(1:2) * Diagonal(1:2) * Diagonal(1:2) * Diagonal(1:2)
2×2 Diagonal{Int64, Vector{Int64}}:
 1   
   16

The 4-term multiplication, as implemented, uses combinations of 2-term multiplications, which returns a Diagonal. Higher number of terms fall back to afoldl, which uses the 2-term multiplication as well. It's only the 3-term multiplication that uses broadcasting and materializes the matrix.

@dlfivefifty
Copy link
Contributor Author

I added a 4-term test so any future regressions will be picked up

@KristofferC KristofferC added the backport 1.9 Change should be backported to release-1.9 label Mar 15, 2023
@dkarrasch dkarrasch added the linear algebra Linear algebra label Mar 15, 2023
@dkarrasch dkarrasch merged commit c37fc27 into JuliaLang:master Mar 16, 2023
oscardssmith pushed a commit to oscardssmith/julia that referenced this pull request Mar 20, 2023
@KristofferC KristofferC mentioned this pull request Mar 24, 2023
52 tasks
KristofferC pushed a commit that referenced this pull request Mar 30, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 31, 2023
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants