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

improve a[...] .= handling of arrays of arrays and dicts of arrays #17568

Merged
merged 3 commits into from
Jul 27, 2016

Conversation

stevengj
Copy link
Member

My ideal in #17510 was to have x .= f.(y) be a synonym for broadcast!(f, x, y). However, because slices return copies and not views, in #17546 I had to modify this so that x[...] .= f.(y) turns into broadcast!(f, view(x, ...), y). This PR extends that solution so that in-place semantics are achieved for arrays of arrays and dictionaries of arrays, to fix a problem pointed out by @iamed2.

In particular, x[...] .= f.(y) now turns into broadcast!(f, Base.dotview(x, ...), y). The dotview function defaults to calling view, but we can override it for dictionaries and arrays-of-arrays to achieve the desired semantics without affecting explicit calls to view.

@iamed2
Copy link
Contributor

iamed2 commented Jul 22, 2016

Hey, thanks! :)

@stevengj
Copy link
Member Author

Once the Arraypocolypse (#13157) lands and slices become views, I think all of this dotview machinery can disappear and we can just use broadcast!(f, x, y) in all cases.

So, hopefully this is all just a temporary hack for Julia 0.5.

@stevengj
Copy link
Member Author

@StefanKarpinski, merge?

@StefanKarpinski
Copy link
Sponsor Member

Seems good to me. I think @JeffBezanson should probably also review it.

@kshyatt kshyatt added the arrays [a, r, r, a, y, s] label Jul 25, 2016
@JeffBezanson
Copy link
Sponsor Member

Would it work to have dotview default to getindex, and only have it call view for AbstractArray arguments? That way we might need fewer ad-hoc definitions, e.g. for Associative.

@stevengj
Copy link
Member Author

Sure.

@stevengj stevengj removed the arrays [a, r, r, a, y, s] label Jul 27, 2016
@stevengj
Copy link
Member Author

stevengj commented Jul 27, 2016

Changed to default to getindex, using view only for AbstractArray as suggested by @JeffBezanson.

@StefanKarpinski
Copy link
Sponsor Member

seems considerably simpler 👍

@JeffBezanson JeffBezanson merged commit e474dd4 into JuliaLang:master Jul 27, 2016
@stevengj stevengj deleted the dotview branch July 27, 2016 16:08

dotview(args...) = getindex(args...)
dotview(A::AbstractArray, args...) = view(A, args...)
dotview{T<:AbstractArray}(A::AbstractArray{T}, args...) = getindex(A, args...)
Copy link
Member

@andreasnoack andreasnoack Feb 1, 2017

Choose a reason for hiding this comment

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

@stevengj Why did you decide to default to getindex and not view in this case? @alanedelman showed me this example today

s = zeros(2,2) 
A = fill(s, 4, 4)
A[1:3,1:3] .= [ones(2,2)]

which doesn't set A because of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, it should be view also. I was thinking that view wasn't necessary, since A[i,j] returns a mutable object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants