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

Fix setindex! on Bidiagonal with ev zero bug. #32328

Merged
merged 1 commit into from
Jul 6, 2019

Conversation

mcognetta
Copy link
Contributor

@mcognetta mcognetta commented Jun 15, 2019

Below is a MWE and explanation of the bug. The bug is that we can assign to structural zeros in Bidiagonal matrices when the B.ev vector is zero, but they show up in the off diagonal. For example:

julia> using LinearAlgebra

julia> B = Bidiagonal(rand(5), zeros(4), 'U')
5×5 Bidiagonal{Float64,Array{Float64,1}}:
 0.73714  0.0                                
         0.800223  0.0                       
                  0.328117  0.0              
                           0.519735  0.0     
                                    0.313492

julia> istril(B)
true

julia> istriu(B)
true

julia> B[2,1] = 3 # Note, this is in the lower off-diagonal but this is an upper Bidiagonal matrix
3

julia> B
5×5 Bidiagonal{Float64,Array{Float64,1}}:
 0.73714  3.0                                
         0.800223  0.0                       
                  0.328117  0.0              
                           0.519735  0.0     
                                    0.313492

julia> B[1,2] = 0
0

julia> B[4,5] = 10
10

julia> B
5×5 Bidiagonal{Float64,Array{Float64,1}}:
 0.73714  0.0                                 
         0.800223  0.0                        
                  0.328117  0.0               
                           0.519735  10.0     
                                     0.313492

Why it happens: In setindex! are checking istriu(B) which is

istriu(M::Bidiagonal) = M.uplo == 'U' || iszero(M.ev)

So, when the Bidiagonal matrix has uplo = 'U' but B.ev is zero, setindex! allows us to set values on the lower off diagonal.

This fix has the added effect of improving access time in the (admittedly, artificial) pessimal case of the very last element in B.ev being the only non-zero one.

@StefanKarpinski
Copy link
Member

@andreasnoack, any chance you could review?

@mcognetta
Copy link
Contributor Author

If you do review, please check #32329 as well (they are both small and very similar).

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.

LGTM

@StefanKarpinski StefanKarpinski merged commit 1272185 into JuliaLang:master Jul 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants