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 .= return its right hand side. fixes #25954 #26088

Merged
merged 1 commit into from
Feb 27, 2018

Conversation

JeffBezanson
Copy link
Member

This is a really simple, sub-optimal solution to try to get the right behavior in place quickly. The hack is to lower .= both ways (the usual broadcast fusion way, and the version that saves the RHS in a temporary variable), and select one at the point where we know whether the expression's value is used.

If we want, the same mechanism could instead be used to give an error for using the value of .=, if we'd rather not decide now.

@JeffBezanson JeffBezanson added broadcast triage This should be discussed on a triage call labels Feb 16, 2018
@JeffBezanson
Copy link
Member Author

Fixes #25954

@stevengj
Copy link
Member

stevengj commented Feb 20, 2018

Since the whole point of .= is to avoid allocating temporaries, defining it to return its LHS makes a lot more sense to me. I don't understand the insistence that it should behave like = in its return value when it behaves differently from = in other ways, anyway.

In what circumstance is returning the RHS, i.e. allocating a temporary that the user presumably chose .= to avoid, a desirable behavior? What practical problem does this PR solve?

@JeffBezanson
Copy link
Member Author

That's a good question. It's hard to say, since there are almost no (maybe actually zero?) examples of code that uses the value of .=.

One possible use is a .= b .= c, where the shapes of a and b are each compatible with c, but not with each other.

Another issue is that e.g. A[:,:] = 0 returns 0, so it's surprising for A .= 0 to return an array.

@stevengj
Copy link
Member

Returning the LHS lets you chain the .= assignment with additional in-place functions like sort!

If you expect the RHS to be broadcasted, it’s not so strange that it changes shape—this is what broadcasting does.

@JeffBezanson
Copy link
Member Author

It can change type as well as shape. In a .= b .= pi, a would be initialized to pi converted to the element type of b, instead of the original pi.

@stevengj
Copy link
Member

stevengj commented Feb 21, 2018

That's true. I still don't see it as a big problem. If someone writes a .= b .= sin.(c) they are clearly trying to avoid allocating temporary arrays. They won't be grateful if we allocate sin.(c) in order to return the RHS to prevent it from passing through conversion to eltype(b).

.= is already fundamentally different from = in lots of ways. Defining it to return the LHS seems much more practically useful, and consonant with the in-place-fusing purpose of .=, than returning the RHS (possibly allocating an unwanted copy).

Defining it to return the RHS seems to fundamentally conflict with the purpose of .=, which is to avoid allocating temporary arrays. I don't think the (largely hypothetical) benefits are worth this price.

@mbauman
Copy link
Member

mbauman commented Feb 21, 2018

It's hard to say, since there are almost no (maybe actually zero?) examples of code that uses the value of .=.

I'm not so sure about that. There are no registered packages with single lines of code with a = b .= c or a .= b .= c, but there are 11 examples of return y .= …, and I'd bet even more implicit returns. And there's also the use of the expression within function calls like the mul!(x, y.*=2) example in the issue thread.

I could get behind an error here.

@stevengj
Copy link
Member

stevengj commented Feb 21, 2018

@mbauman, in all of the examples you cite, it sounds like returning the LHS is the desired behavior. This makes total sense to me, because using .= means "I don't want the RHS to be materialized."

@mbauman
Copy link
Member

mbauman commented Feb 21, 2018

I mean, it is that way because that's the way it is. I do agree that this could result in unexpected allocations and double-execution of a RHS broadcast, which is strange and unfortunate. On the other hand, this seems strange to me:

julia> function f(x)
           local a::Any
           local b::Int
           a = b = x
           (a, b)
       end
f (generic function with 1 method)

julia> f(2.)
(2.0, 2)

julia> function g(x)
           a = Array{Any}(uninitialized, 1)
           b = Array{Int}(uninitialized, 1)
           @. a = b = x
           (a, b)
       end
g (generic function with 1 method)

julia> g([2.])
(Any[2], [2])

@JeffBezanson
Copy link
Member Author

From triage: I'll add a deprecation warning for this (and therefore not change the behavior), since that would be the usual course of action anyway, and then we can decide later whether to change it or turn the warning into an error.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Feb 22, 2018
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Feb 22, 2018

There are two cases essentially:

  1. a = b .= f.(c, d)
  2. a .= b .= f.(c, d)

In the first case, b already captures the value of the LHS of the .= so returning that buys nothing – the only thing you might want is to capture the RHS but also assign into b in place. In the second case, there's no reason why any temporary would need to be allocated: in place assignment into a and b could be done minimally by evaluating the right loops. So I find the argument for returning the LHS to avoid unnecessary temporaries unconvincing – returning the LHS is useless in one case and no more efficient in the other case.

On the other hand, evaluating the RHS is consistent with the behavior of =. It also makes case 1 above actually useful instead of redundant since a and b can capture different things (and could arguably be written more clearly as b .= (a = f.(c, d)) assuming that would actually fuse). @mbauman's argument above shows why evaluating to the LHS in case 2 would be confusing in that the type of b would affect the value of a via spooky action at a distance. This situation is no different than the situation for a = b = f(...). Neither the LHS nor the RHS evaluation behavior actually inherently entails more allocation and evaluating to the RHS is more consistent with the rest of the language and has less potential for spooky action at a distance hazard.

Overall, I have a hard time seeing valid arguments for returning the LHS versus the RHS.

@stevengj
Copy link
Member

stevengj commented Feb 22, 2018

If you define:

function f!(x)
     x .= foo.(x)
end

shouldn't it return the LHS (x) rather than allocating another array and returning foo.(x)?

If you do a function call

sort!(x .= x.^2)

shouldn't it call sort! on x, and not on a new array of x.^2?

These, to me, are more realistic uses of the value of .=, rather than explicit a = b .= c or a .= b .= c expressions. Returning the RHS defeats the purpose of using .=.

@mbauman
Copy link
Member

mbauman commented Feb 22, 2018

I just have a hard time seeing how that's any different from:

julia> function f()
           local x::Int
           x = 2.0
       end
f (generic function with 1 method)

julia> f()
2.0

Or:

julia> function settwototwo!(x)
           x[2] = 2
       end
settwototwo! (generic function with 1 method)

julia> settwototwo!([4,3,2])
2

@stevengj
Copy link
Member

Because the whole point of .=, almost the only reason to use it, is to avoid allocating the right-hand-side? Because it is defined to be sugar for broadcast!?

The question is, are there any practical circumstances where the users of .= will be happy if you allocate the RHS in order to return it? Or will basically all users of .= be annoyed by this behavior?

Would you be happy if you wrote return x .= sin.(y) or sort!(x .= sin.(y)) and it returned a new array rather than x?

@JeffBezanson
Copy link
Member Author

It just means you need to write x .= sin.(y); return x instead of return x .= sin.(y).

@stevengj
Copy link
Member

Sure, there is a workaround. There is also a workaround if you want the RHS. That doesn’t address the question of which behavior is more useful.

@JeffBezanson
Copy link
Member Author

That's true, but it's hard to guess what we would find more useful, so we need to have simple rules like "assignments always return the RHS".

Anyway, I believe this is used quite rarely, so I'll proceed with just deprecating it. That way there's no final decision yet.

@JeffBezanson JeffBezanson merged commit 4fe912b into master Feb 27, 2018
@JeffBezanson JeffBezanson deleted the jb/chaineddotequals branch February 27, 2018 22:12
@mbauman
Copy link
Member

mbauman commented Mar 1, 2018

I'm getting false positives in #24368 with code like this:

$ ./julia --depwarn=error -q
julia> module TestFoo
           x .= y
           nothing
       end
ERROR: syntax: Deprecated syntax `using the value of `.=``.

mbauman added a commit that referenced this pull request Mar 5, 2018
…gnment

* master:
  Make stdlib tests runnable standalone. (#26242)
  fix unary-related parsing regressions caused by #26154 (#26239)
  Formatting changes to new SSA IR devdocs [ci skip]
  use medium code model on PPC
  `retry` should support the check function proposed in the docstring. (#26138)
  mention axes in docs instead of size (#26233)
  exclude more CI files from source distro (#25906)
  Describe three-valued logic in docstrings
  deprecate using the value of `.=`. fixes #25954 (#26088)
  backport change to make CodegenPrepare work with isNoopAddrSpaceCast
  optimize the python version of abs2 in the microbenchmarks (#26223)
  Add notes for package maintainers (#25912)
  typo
  Fix broken links to functions in the manual (#26171)
  [NewOptimizer] Track inlining info
  Begin work on new optimizer framework
  add patch to make GC address spaces work on PPC
  also backport sover patch to LLVM 4.0
@mbauman mbauman added the broadcast Applying a function over a collection label Apr 24, 2018
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.

4 participants