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

Structured matrices don't extend missing dimensions correctly in broadcasting #1063

Open
jishnub opened this issue Apr 15, 2024 · 1 comment · May be fixed by JuliaLang/julia#54190
Open
Labels
broadcast Applying a function over a collection bug Something isn't working

Comments

@jishnub
Copy link
Contributor

jishnub commented Apr 15, 2024

julia> D1 = Diagonal([1]); D2 = Diagonal([1,2]);

julia> D1 .+ D2
2×2 Diagonal{Int64, Vector{Int64}}:
 2  
   3

julia> Matrix(D1) .+ D2
2×2 Matrix{Int64}:
 2  1
 1  3

Ideally, these should produce identical results.
In the structured case, the 1x1 matrix D1 isn't expanded to ones(size(D2)), but instead it's expanded to Diagonal(ones(Int, size(D2,1))). I'm unsure if there's a way to resolve this while preserving type-stability.

@N5N3 N5N3 added the bug Something isn't working label Apr 16, 2024
@mbauman
Copy link
Member

mbauman commented Apr 16, 2024

Oh, yikes. The problem is in similar, but I'm not sure what to do about it:

https://github.com/JuliaLang/julia/blob/e5821b3220bb66d18beb2fcae70d40779ba737c3/stdlib/LinearAlgebra/src/structuredbroadcast.jl#L155-L166

julia> bc = Broadcast.instantiate(Broadcast.broadcasted(+, D1, D2))
Base.Broadcast.Broadcasted{LinearAlgebra.StructuredMatrixStyle{Diagonal}}(+, ([1;;], [1 0; 0 2]))

julia> similar(bc, Int)
2×2 Diagonal{Int64, Vector{Int64}}:
 4333067184           ⋅
          ⋅  4333069200

I think you're right that this will need to break type stability. It could alternatively be an error, but that also seems quite broken. This probably happens with all the structured matrices.

@dkarrasch dkarrasch added the broadcast Applying a function over a collection label Apr 17, 2024
N5N3 referenced this issue in N5N3/julia Apr 22, 2024
1. size-1 StructuredMatrix should behave like scalar during broadcast. Thus their `fzero` should return the only element.
(fix #54087)

2. But for simple broadcast with only one StructuredMatrix, we can keep stability as the structure is "preserved" even for size-1 case. Thus `count_structedmatrix` is added to check that.

3. `count_structedmatrix` is fused to keep `Bidiagonal` stability.
(replace JuliaLang#54067)
@KristofferC KristofferC transferred this issue from JuliaLang/julia Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants