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 array .= silently ignores unstructured non-zeros #31674

Closed
ararslan opened this issue Apr 10, 2019 · 5 comments
Closed

Structured array .= silently ignores unstructured non-zeros #31674

ararslan opened this issue Apr 10, 2019 · 5 comments
Labels
arrays [a, r, r, a, y, s] broadcast Applying a function over a collection linear algebra Linear algebra

Comments

@ararslan
Copy link
Member

Consider the following:

julia> U = UpperTriangular(Float64[1 2 3; 0 1 2; 0 0 1])
3×3 UpperTriangular{Float64,Array{Float64,2}}:
 1.0  2.0  3.0
  ⋅   1.0  2.0
  ⋅    ⋅   1.0

julia> X = randn(3, 3)
3×3 Array{Float64,2}:
  0.9189    -0.942656  -0.825467
 -0.346463   0.829813   0.741945
 -0.239974  -0.232412   1.46933 

julia> U .+= X
3×3 UpperTriangular{Float64,Array{Float64,2}}:
 1.9189  1.05734  2.17453
  ⋅      1.82981  2.74194
  ⋅       ⋅       2.46933

Note that U was updated with the result of elementwise addition with X. However, U is still an UpperTriangular, even though the right hand side of the addition is not. Generally speaking, the result of adding two matrices is only upper triangular if both are.

It seems to me that the case of updating a structured matrix should not work except when the operation would produce a matrix of the same structure.

@ararslan ararslan added linear algebra Linear algebra arrays [a, r, r, a, y, s] broadcast Applying a function over a collection labels Apr 10, 2019
@mcognetta
Copy link
Contributor

Seems like the most practical solution is to just disallow this with structured and non-structured arguments. Otherwise it would have to test for triangularity, diagonality, etc each time.

@ararslan
Copy link
Member Author

Otherwise it would have to test for triangularity, diagonality, etc each time.

I'm not sure what you mean; we could simply whitelist types in a method signature so that dispatch knows which types it can use.

@mbauman
Copy link
Member

mbauman commented Apr 10, 2019

Ah, dang. This is an issue for all structured array assignments. All those methods need to check for structure/zero-preserving-ness like the non-in-place broadcasts.

@mbauman mbauman changed the title Structured array .+= different array works but is wrong Structured array .= silently ignores unstructured non-zeros Apr 10, 2019
@mcognetta
Copy link
Contributor

Sorry, we are on the same page. I misread this part:

It seems to me that the case of updating a structured matrix should not work except when the operation would produce a matrix of the same structure.

What I meant by this was that if we allowed any matrix type, then we would have to call istriu or whatever each time, which would be expensive in the non-structured cases.

A small problem is that we lose some cases where we have (for example) a LowerTriangular matrix that is actually diagonal as well.

mbauman added a commit that referenced this issue Apr 10, 2019
Previously, broadcasted assignment (`.=`) would happily ignore all nonstructured portions of the destination, regardless of whether the broadcasted expression would actually evaluate to zero or not. This changes these in-place methods to use the same infrastructure that out-of-place broadcast uses to determine the result type. If we are unsure of the structural properties of the output, we fall back to the generic implementation, which will attempt to store into every single location of the destination -- including those structural zeros. Thus we now error in cases where we generate nonzeros in those locations.
@mbauman
Copy link
Member

mbauman commented Apr 10, 2019

Note that it's not just argument types — it also depends on the function. For example, X .= sin.(X) works but X .= cos.(X) should not for all the structured matrices. We have this infrastructure, it just needed to be extended to the in-place methods.

#31678

@KristofferC KristofferC mentioned this issue Apr 15, 2019
58 tasks
KristofferC pushed a commit that referenced this issue May 20, 2019
…#31678)

Previously, broadcasted assignment (`.=`) would happily ignore all nonstructured portions of the destination, regardless of whether the broadcasted expression would actually evaluate to zero or not. This changes these in-place methods to use the same infrastructure that out-of-place broadcast uses to determine the result type. If we are unsure of the structural properties of the output, we fall back to the generic implementation, which will attempt to store into every single location of the destination -- including those structural zeros. Thus we now error in cases where we generate nonzeros in those locations.

(cherry picked from commit 6bd3967)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] broadcast Applying a function over a collection linear algebra Linear algebra
Projects
None yet
Development

No branches or pull requests

3 participants