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

Address part of #28866, prevent array operations from dropping 0d containers #32122

Merged
merged 3 commits into from
Jul 3, 2019

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented May 23, 2019

This is a simple workaround for the handful of elementwise operations that are defined on arrays without the need for explicit broadcast but use broadcasting (with an extra shape check) in their implementation. These were the only affected cases I could find.

This is restoring the intended behavior, and matches the behavior of Julia pre-0.7. It is a fairly impactful change, though, so even though I'd call it a bugfix we probably want to be conservative in our backporting.

@mbauman mbauman added the arrays [a, r, r, a, y, s] label May 23, 2019
@vtjnash vtjnash added the triage This should be discussed on a triage call label May 27, 2019
@StefanKarpinski StefanKarpinski added the minor change Marginal behavior change acceptable for a minor release label Jun 6, 2019
@StefanKarpinski
Copy link
Member

Seems reasonable. I think whatever you end up deciding here is good, @mbauman.

mbauman added a commit that referenced this pull request Jun 10, 2019
@mbauman mbauman changed the title Fix #28866, prevent array operations from dropping 0d containers Address part of #28866, prevent array operations from dropping 0d containers Jun 10, 2019
mbauman and others added 2 commits June 10, 2019 19:24
…tainers

This is a simple workaround for the handful of elementwise operations that are defined on arrays _without_ the need for explicit broadcast but use broadcasting (with an extra shape check) in their implementation. These were the only affected cases I could find.
@mbauman
Copy link
Member Author

mbauman commented Jun 10, 2019

Failures were real and stemmed from the fact that LinearAlgebra specialized broadcast for adjoint/transposed vectors but didn't hook into the broadcast machinery. I initially tried to start addressing the root cause and move the specializations to the equivalent copy(::Broadcasted{...}) methods, but that ended up with effective ambiguities between this behavior and SparseArray's PromoteToSparse style. It'd be good to address this in the future but I wanted to keep this change as minimal as possible. So see #32289 for that change.

@StefanKarpinski
Copy link
Member

Look at all those green checks 😁

@mbauman
Copy link
Member Author

mbauman commented Jul 3, 2019

I say we do this.

@StefanKarpinski StefanKarpinski merged commit 38b38cf into master Jul 3, 2019
@StefanKarpinski StefanKarpinski deleted the mb/28866 branch July 3, 2019 19:30
@mbauman mbauman removed the triage This should be discussed on a triage call label Jul 3, 2019
KristofferC added a commit that referenced this pull request Aug 26, 2019
KristofferC added a commit that referenced this pull request Aug 26, 2019
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] minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants