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 #31674, error when storing nonzeros into structural zeros with .= #31678

Merged
merged 2 commits into from
May 16, 2019

Conversation

mbauman
Copy link
Sponsor Member

@mbauman mbauman commented 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.

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 mbauman added domain:linear algebra Linear algebra kind:bugfix This change fixes an existing bug domain:broadcast Applying a function over a collection backport 1.2 labels Apr 10, 2019
T = Tridiagonal(rand(N - 1), rand(N), rand(N - 1))
◣ = LowerTriangular(rand(N,N))
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@andreasnoack
Copy link
Member

Why is it necessary to add special checks for this instead? I would assume that, in general, in-place broadcasting would automatically fail when there are structural zeros with only whitelisted special cases (zero preserving mapings) would work.

@mbauman
Copy link
Sponsor Member Author

mbauman commented Apr 11, 2019

Yes, the general broadcast methods just do the right thing. An alternative PR would be to simply remove all these copyto! methods.

But these copyto! methods are huge optimizations! They only store into the structural nonzeros (typically O(n)) and completely ignore all the calculations of all the zeros (typically O(n^2)). Of course we can only perform these optimizations when its safe. Thus the checks. If a given broadcast expression is not known to be safe, then we punt out to the general broadcast method (which is of course just O(n^2)).

@KristofferC KristofferC mentioned this pull request Apr 15, 2019
58 tasks
@JeffBezanson
Copy link
Sponsor Member

Should this be merged?

@mbauman
Copy link
Sponsor Member Author

mbauman commented May 16, 2019

Yes please! This is good by me; I'm probably just overly conservative about waiting for express approval before merging my own PRs.

@JeffBezanson JeffBezanson merged commit 6bd3967 into master May 16, 2019
@JeffBezanson JeffBezanson deleted the mb/fix31674 branch May 16, 2019 22:30
KristofferC pushed a commit that referenced this pull request 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
domain:broadcast Applying a function over a collection domain:linear algebra Linear algebra kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants