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 type of allocated array when broadcasting type unstable function #37028

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

nalimilan
Copy link
Member

We need to call similar on the Broadcasted object rather than on dest array. Otherwise the BroadcastStyle isn't taken into account when allocating new array due to function returning elements of different types.

See #36106 (comment).

We need to call similar on the `Broadcasted` object rather than on dest array.
Otherwise the `BroadcastStyle` isn't taken into account when allocating new
array due to function returning elements of different types.
@nalimilan nalimilan requested a review from mbauman August 13, 2020 11:09
fprod(aa) = aa .* aa'
@test a .+ 1 == @inferred(fadd(aa))
@test a .+ 1 .* 2 == @inferred(fadd2(aa))
@test a .* a' == @inferred(fprod(aa))
@test isequal(a .+ [missing; 1:9], fadd3(aa))
@test_broken Core.Compiler.return_type(fadd3, (typeof(aa),)) <: Array19745{<:Union{Float64, Missing}}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is #30485.

@mbauman
Copy link
Member

mbauman commented Aug 13, 2020

Ah, this makes sense. So the destination is initially created with:

dest = similar(bc, T)

Subsequent widenings had previously been

dest = similar(dest, T′)

but that privileges the initial similar(bc, T) and makes things order-dependent. Always going back to the bc object here definitely seems to be the right thing to do.

@mbauman mbauman added the broadcast Applying a function over a collection label Aug 13, 2020
@JeffBezanson
Copy link
Member

Any chance somebody could trip over this? Maybe a NEWS item?

@nalimilan
Copy link
Member Author

I don't think it should surprise anybody. If this changes the behavior for you, then you obtained different array (container) types depending on whether your function returned elements of the same type or not. Package authors that experienced this are more likely to have worked around this by defining similar methods on the array type in addition to methods on Broadcasted (like I did on CategoricalArrays, among other reasons). Also it's so technical that probably nobody will understand the meaning the a NEWS.md entry mentioning it. :-p

@JeffBezanson JeffBezanson merged commit 327b2e3 into master Aug 17, 2020
@JeffBezanson JeffBezanson deleted the nl/broadcast branch August 17, 2020 20:09
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 29, 2020
…uliaLang#37028)

We need to call similar on the `Broadcasted` object rather than on dest array.
Otherwise the `BroadcastStyle` isn't taken into account when allocating new
array due to function returning elements of different types.
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.

3 participants