Skip to content

Conversation

ssikdar1
Copy link
Contributor

@ssikdar1 ssikdar1 commented Mar 27, 2020

Description

Tries to Fix JuliaLang/LinearAlgebra.jl#707
Adds a check for when theres two Bidiagnoals with differing uplo types.
In this case a Tridiagnoal should be returned. In the case that uplo is the same, a Bidiagnoal is still returned. ( and all other cases stay the same )

Testing

added unit tests and ran structuredbroadcast.jl

$ ./julia  stdlib/LinearAlgebra/test/structuredbroadcast.jl
Test Summary:                                                                              | Pass  Total
broadcast[!] over combinations of scalars, structured matrices, and dense vectors/matrices |  240    240
Test Summary:                                           | Pass  Total
broadcast! where the destination is a structured matrix |   26     26
Test Summary:                                   | Pass  Total
map[!] over combinations of structured matrices |  288    288
Test Summary: | Pass  Total
Issue JuliaLang/julia#33397  |   10     10
Test Summary:            | Pass  Total
Broadcast Returned Types |    4      4

Also testing in the shell:

  julia> using LinearAlgebra

julia> Bidiagonal(ones(4), ones(3), :U) .+ Bidiagonal(ones(4), ones(3), :L)
4×4 Tridiagonal{Float64,Array{Float64,1}}:
 2.0  1.0   ⋅    ⋅ 
 1.0  2.0  1.0   ⋅ 
  ⋅   1.0  2.0  1.0
  ⋅    ⋅   1.0  2.0

julia> Bidiagonal(ones(4), ones(3), :L) .+ Bidiagonal(ones(4), ones(3), :U)
4×4 Tridiagonal{Float64,Array{Float64,1}}:
 2.0  1.0   ⋅    ⋅ 
 1.0  2.0  1.0   ⋅ 
  ⋅   1.0  2.0  1.0
  ⋅    ⋅   1.0  2.0

  julia> Bidiagonal(ones(4), ones(3), :U) .+ Bidiagonal(ones(4), ones(3), :U)
4×4 Bidiagonal{Float64,Array{Float64,1}}:
 2.0  2.0   ⋅    ⋅ 
  ⋅   2.0  2.0   ⋅ 
  ⋅    ⋅   2.0  2.0
  ⋅    ⋅    ⋅   2.0


julia> Bidiagonal(ones(4), ones(3), :L) .+ Bidiagonal(ones(4), ones(3), :L)
4×4 Bidiagonal{Float64,Array{Float64,1}}:
 2.0   ⋅    ⋅    ⋅ 
 2.0  2.0   ⋅    ⋅ 
  ⋅   2.0  2.0   ⋅ 
  ⋅    ⋅   2.0  2.0

@ssikdar1 ssikdar1 changed the title add bidiagnoal U/L check in broadcast Fix Broadcasting of Bidiagonal Mar 28, 2020
@dkarrasch dkarrasch added broadcast Applying a function over a collection linear algebra Linear algebra labels Mar 28, 2020
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

Very nice. Just a minor layout comment and a suggestion to strengthen the tests further.

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

I'm not a broadcasting expert, but the desired tests pass.

@dkarrasch dkarrasch requested a review from mbauman March 29, 2020 17:48
@mbauman
Copy link
Member

mbauman commented Mar 30, 2020

Thanks so much for taking a stab at this! I think we want a slightly more comprehensive solution, though — this will only work for exactly adjacent pairings of Bidiagonals, but fail if there are more arguments or nesting involved. The key will be to do something like how find_bidiagonal works, except I think it should be changed from short-circuiting at the first bidiagonal to combining their uplos. We could return 'U' or 'L' (if homogenous) or missing if heterogeneous.

@ssikdar1
Copy link
Contributor Author

@mbauman Apologies if this is sloppy, but is 9510c3a what you were thinking?
I've created a find_uplo function that has logic similar to the find_bidiagnoal.

@dkarrasch dkarrasch closed this Jun 8, 2020
@dkarrasch dkarrasch reopened this Jun 8, 2020
@dkarrasch dkarrasch merged commit 0e062e9 into JuliaLang:master Jun 8, 2020
@mbauman
Copy link
Member

mbauman commented Jun 8, 2020

Thanks @dkarrasch, I forgot about this one. Let's backport it, too.

@mbauman mbauman added backport 1.5 bugfix This change fixes an existing bug labels Jun 8, 2020
KristofferC pushed a commit that referenced this pull request Jun 16, 2020
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
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 bugfix This change fixes an existing bug linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broadcasting of Bidiagonal incorrectly returns Bidiagonal
4 participants