Skip to content

Make .= work for arrays of arrays by returning a view from Base.dotview #20379

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

Merged
merged 1 commit into from
Feb 5, 2017

Conversation

andreasnoack
Copy link
Member

@andreasnoack andreasnoack commented Feb 2, 2017

This was motivated by the example I've added as a test. This also changes the behavior of a[1] .= when the left-hand side is an array of arrays as can be seen from the tests I've commented out. I'm not sure about the behavior in this case. Part of me can see the point in the old behavior and part of me thinks it is wrong to let the broadcast behavior go one level down in the scalar case instead of considering the element a unit. One idea was I had was to restrict the definition I've now removed to

dotview{T<:AbstractArray}(A::AbstractArray{T}, args::Integer...) = getindex(A, args...)

but this wouldn't be correct for arrays with custom indices. Alternatively, I wondered if (a[1]) .= should parse differently such that the getindex is applied before the broadcast!.

@mbauman
Copy link
Member

mbauman commented Feb 2, 2017

One possible option would be to re-introduce that special behavior in broadcast!(::AbstractArray{T,0} where T, …). Instead of using a shape of (), it'd use the shape of the element. This should generalize nicely for Array{Any,0}.

@stevengj stevengj added needs decision A decision on this change is needed broadcast Applying a function over a collection breaking This change will break code labels Feb 2, 2017
@stevengj
Copy link
Member

stevengj commented Feb 2, 2017

We can't apply getindex before the broadcast, because for e.g. Array{Int} there will be no way to mutate the result of getindex.

What's wrong with dotview{T<:AbstractArray}(A::AbstractArray{T}, args::Integer...) = getindex(A, args...)? (If the indices aren't integers, then it isn't really an AbstractArray, and I don't think broadcast would work anyway.)

@andreasnoack
Copy link
Member Author

andreasnoack commented Feb 2, 2017

With http://docs.julialang.org/en/release-0.5/devdocs/offset-arrays/#arrays-with-custom-indices, I didn't think that indices had to be Integers anymore. @timholy Am I wrong and should that be part of the documentation?

@stevengj
Copy link
Member

stevengj commented Feb 3, 2017

Even if there are some weird non-integer-indexed arrays in the future, dotview{T<:AbstractArray}(A::AbstractArray{T}, args::Integer...) = getindex(A, args...) still seems worth adding now.

@andreasnoack
Copy link
Member Author

I'm fine with that approach. We can revisit if it becomes a problem. I'll push the change in a moment.

@andreasnoack andreasnoack merged commit 6032bc6 into master Feb 5, 2017
@andreasnoack andreasnoack deleted the anj/dotview branch February 5, 2017 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code broadcast Applying a function over a collection needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants