Skip to content

Make op(Array, Scalar) use broadcast#19692

Merged
andreasnoack merged 1 commit intomasterfrom
anj/op
Dec 28, 2016
Merged

Make op(Array, Scalar) use broadcast#19692
andreasnoack merged 1 commit intomasterfrom
anj/op

Conversation

@andreasnoack
Copy link
Copy Markdown
Member

@andreasnoack andreasnoack commented Dec 23, 2016

These are broadcast operations and could therefore just use the broadcast functionality. @martinholters I had to tighten a single type test that you added but I believe that this can just be considered an improvement.

@stevengj
Copy link
Copy Markdown
Member

The original thinking was that, for example, we shouldn't widen Array{Float32} even when multiplying it by a Float64. But in cases where this matters, one probably wants to use the (new) in-place operations anyway, so this seems like a reasonable change to me.

Needs a NEWS item under breaking changes, though.

@martinholters
Copy link
Copy Markdown
Member

martinholters commented Dec 23, 2016

I had to tighten a single type test that you added but I believe that this can just be considered an improvement.

I had the same change in the branch mentioned in #19669, so 👍.

@kshyatt kshyatt added the broadcast Applying a function over a collection label Dec 24, 2016
@kshyatt
Copy link
Copy Markdown
Member

kshyatt commented Dec 27, 2016

Can we merge this?

@andreasnoack
Copy link
Copy Markdown
Member Author

Still needs an updated NEWS.md. Will do that now.

@andreasnoack
Copy link
Copy Markdown
Member Author

Done.

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Dec 27, 2016

@nanosoldier runbenchmarks(ALL, vs = ":master")

@KristofferC
Copy link
Copy Markdown
Member

So code like f(x::Vector) = x[1] < 0.5 ? x[1] * x : 0.1 * x will now be type unstable for Vector{Float32}?

@andreasnoack
Copy link
Copy Markdown
Member Author

Yes. Unless we change the behavior of broadcast promotion. I don't really think it makes sense to have different promotion behavior between these and broadcast. The main point here is to make the two similar but I also think it makes more sense to follow the scalar-scalar promotion rules. The function is type unstable in the same way f(x::Number) = x < 0.5 ? x*x : 0.1*x is type unstable for Float32.

@nanosoldier
Copy link
Copy Markdown
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@andreasnoack
Copy link
Copy Markdown
Member Author

Couldn't reproduce the regression locally so I assume it is noise.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants