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

make x[...] .= ... assign in-place #17546

Merged
merged 4 commits into from
Jul 22, 2016
Merged

Conversation

stevengj
Copy link
Member

This fixes a bug in #17510: x[...] .= ... and x[...] .+= ... etcetera should call broadcast! on view(x, ...), so that it continues to work in-place. (In Julia 0.4, x[...] .+= ... translates into a setindex! call.)

@stevengj
Copy link
Member Author

(@timholy, is there anything about the current view implementation that would prevent broadcast!(identity, view(x, ...), rhs) from being a drop-in replacement for setindex!(x, rhs, ...)? It seems to work in all the cases I've tried.)

(let* ((ex (partially-expand-ref expr))
(stmts (butlast (cdr ex)))
(refex (last (cdr ex)))
(nuref `(call view ,(caddr refex) ,@(cdddr refex))))
Copy link
Member Author

Choose a reason for hiding this comment

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

@JeffBezanson, should this be (top view) rather than view?

Copy link
Member

Choose a reason for hiding this comment

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

Yes; I would go with top to avoid capturing locals that might be called view or broadcast.

@stevengj stevengj added this to the 0.5.0 milestone Jul 22, 2016
@stevengj
Copy link
Member Author

stevengj commented Jul 22, 2016

Marking as 0.5, since this needs to be resolved one way or the other (either by merging this in some form or by reverting #17510) by 0.5. There is code in existing packages that uses x[...] .+= ... and expects in-place updates.

@stevengj stevengj added the regression Regression in behavior compared to a previous version label Jul 22, 2016
@stevengj
Copy link
Member Author

Okay to merge?

@tkelman
Copy link
Contributor

tkelman commented Jul 22, 2016

would be good to get a confirmation on #17546 (comment) ?

@stevengj
Copy link
Member Author

@tkelman, even if there is some case where view is problematic, I think that this PR is a strict improvement on the broken behavior we have currently (my fault!).

If view fails, my suspicion is that it will fail noisily — most likely a MethodError, if there is some custom array-like type that implements an odd getindex method but not a corresponding view method. (e.g. PyCall uses getindex in a weird way, and view is not implemented for PyObject. I'll have to think about whether .= even makes sense for a PyObject.)

@StefanKarpinski
Copy link
Member

Seems like sound reasoning, @stevengj – in favor of merging.

@stevengj stevengj merged commit 55623a1 into JuliaLang:master Jul 22, 2016
@stevengj stevengj deleted the dotassign-ref branch July 22, 2016 14:55
@stevengj
Copy link
Member Author

One case that we can't handle yet is that of a dictionary. Neither view nor broadcast! currently handle dictionaries, so something like mydict[key] .*= ... will throw a MethodError for now.

One way to handle this would be to implement a view(a::Associative, key) method that just returns a[key], I think. That way, if a[key] is a mutable array then a[key] .= ... will work as expected, while if a[key] is an immutable object then broadcast! will throw a MethodError.

@iamed2
Copy link
Contributor

iamed2 commented Jul 22, 2016

I think people are going to be confused when .= works when LHS is an array but not when LHS returns an array. Another case is:

julia> a = [[4, 5], [6, 7]]
julia> a[1] .= 3
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Array{Int64,1}
This may have arisen from a call to the constructor Array{Int64,1}(...),
since type constructors fall back to convert methods.
 in fill!(::SubArray{Array{Int64,1},0,Array{Array{Int64,1},1},Tuple{Int64},false}, ::Int64) at ./multidimensional.jl:549
 in broadcast!(::Base.#identity, ::SubArray{Array{Int64,1},0,Array{Array{Int64,1},1},Tuple{Int64},false}, ::Int64) at ./broadcast.jl:19
 in eval(::Module, ::Any) at ./boot.jl:234
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##3#4{Base.REPL.REPLBackend})() at ./event.jl:46

The way to get the intended result is:

julia> a[1][:] .= 3
2-element SubArray{Int64,1,Array{Int64,1},Tuple{Colon},true}:
 3
 3
julia> a
2-element Array{Array{Int64,1},1}:
 [3,3]
 [6,7]

but that's not exactly obvious and might require a manual entry (unless a special case is made for scalar indexing).

AFAIK this is why MATLAB cell arrays have two types of indexing brackets: () to slice and {} to extract (not that I'm suggesting that).

@stevengj
Copy link
Member Author

stevengj commented Jul 22, 2016

@iamed2, I think that most cases of interest could be supported by adding appropriate methods to broadcast! and/or view.

e.g. your particular case could be handled by defining:

Base.broadcast!{T<:AbstractArray}(f, x::SubArray{T,0}, args...) = broadcast!(f, x[], args...)
Base.broadcast!{T<:AbstractArray}(f::typeof(identity), x::SubArray{T,0}, arg::Number) = broadcast!(f, x[], arg) # prevent method ambiguity

We could also get more fine-grained control by defining a broadcastview(...) function, which defaults to view(...), to allow us to define behaviors that only affect the implicit views from .= and not explicit calls to view.

@stevengj
Copy link
Member Author

Note also that this PR can be reverted in 0.6 if the arraypocolypse happens and slices become views. So potentially this is just a temporary fix.

stevengj added a commit to stevengj/julia that referenced this pull request Jul 22, 2016
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
* make x[...] .= ... assign in-place (fixes bug in JuliaLang#17510)

* doc fix

* You're the top! You're the Coliseum.

* better var name
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants