-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Deprecate +
/-
methods for array+scalar
etc
#22932
Conversation
xref #22880 |
While +(matrix, scalar) is usually more a source of bugs than anything, +(vector, scalar), while wrong in the linear algebra sense, looks pretty harmless to me, and can sometimes be useful (not every array is used as a vector in linear algebra, after all). If +(vector, scalar) is too inconvenient to remove, a middle ground could be defining it only for rank-1 arrays. |
+
/-
methods for array+scalar
etc+
/-
methods for array+scalar
etc
83868be
to
222230e
Compare
NEWS.md
Outdated
@@ -221,6 +221,10 @@ Deprecated or removed | |||
* The function `showall` is deprecated. Showing entire values is the default, unless an | |||
`IOContext` specifying `:limit=>true` is in use ([#22847]). | |||
|
|||
* Automatically broadcasted `+` and `-` for `array + scalar`, `scalar - array`, and so-on have | |||
been deprecated due to inconsistency with linear algebra. Use `.+` and `.-` for these operations | |||
instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should reference the PR number here like the other bullet points do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For extra credit reference the commit that the NEWS change is made in.
Previous attempt was: #5807, #5810, reverted in #7226. The main difference, as @andyferris says, is that now we've eliminated most of the implicit broadcasting in favor of dot calls, and dot calls have some performance advantages (when they are combined) so it is not merely a matter of spelling. |
base/deprecated.jl
Outdated
@deprecate +(a::Number, b::AbstractArray) a .+ b | ||
@deprecate +(a::AbstractArray, b::Number) a .+ b | ||
@deprecate -(a::Number, b::AbstractArray) a .- b | ||
@deprecate -(a::AbstractArray, b::Number) a .- b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a note here indicating that when these deprecations get removed, we should rewrite these methods throwing a MethodError
that suggests using the dot version and explaining why it fails? (I imagine this could be regular pitfall for a lot of people)
268154f
to
d057b36
Compare
@@ -358,8 +358,6 @@ GeneralPeriod = Union{Period, CompoundPeriod} | |||
|
|||
for op in (:+, :-) | |||
@eval begin | |||
($op)(x::GeneralPeriod, Y::StridedArray{<:GeneralPeriod}) = broadcast($op, x, Y) | |||
($op)(Y::StridedArray{<:GeneralPeriod}, x::GeneralPeriod) = broadcast($op, Y, x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these would need deprecating too, GeneralPeriod isn't a subtype of Number right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
test/ranges.jl
Outdated
@test isa(1 - r - 1, typeof(r)) | ||
@test 1 .+ r .+ (-1) == r | ||
@test 1 .+ collect(r) == collect(1 .+ r) == collect(r .+ 1) | ||
@test_broken isa(1 .+ r .+ (-1), typeof(r)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really broken so much as not done by broadcast, since it's tough in general to know whether an arbitrarily fused operation preserves spacing of a range
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I've been playing with this to make the test better... I just realized there was no 1 .+ r
specialization either, for ranges. (In fact, there's a couple other strange definitions there I will deal with, like 3 / (1:3)
still exists whereas 3 / [1,2,3]
has already been removed in a previous vectorized operations purge).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh god, I'm in some kind of hell where (a) I can't use stacktraces to figure out why my sysimg won't build and (b) we still have the v0.6 deprecations and some of my new range broadcasts are somehow being taken over by the old .+
operator or something...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I can confirm that 1 .+ (1:10)
does not call broadcast(+, 1, 1:10)
on v0.7.
Wow.
d057b36
to
c50c41c
Compare
base/abstractarray.jl
Outdated
@@ -1256,7 +1256,7 @@ function _cat(A, shape::NTuple{N}, catdims, X...) where N | |||
for x in X | |||
for i = 1:N | |||
if concat[i] | |||
inds[i] = offsets[i] + cat_indices(x, i) | |||
inds[i] = broadcast(+, offsets[i], cat_indices(x, i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone is curious, this one was a pain to find...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use .+
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was scared of what I think turned out to the issue of literals "disguising" the broadcasted function. I really should go through this PR and revert calls like this to .+
everywhere literals aren't used.
328a653
to
2db93d0
Compare
base/range.jl
Outdated
# also, separate in case of noncommutative multiplication (division) | ||
broadcast(::typeof(\), x::Number, r::Range) = range(x\first(r), x\step(r), x\length(r)) | ||
broadcast(::typeof(\), x::Number, r::StepRangeLen) = StepRangeLen(x\r.ref, x\r.step, length(r), r.offset) | ||
broadcast(::typeof(\), x::Number, r::LinSpace) = LinSpace(x \ r.start, x \ r.stop, r.len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all broadcast(f, ...)
for f
= {*
, /
, \
} methods really needed? Either literals or broadcast fusion can defeat the purpose of this definition returning and array iterating over each element of the range. I'd say that is why we have *
, /
and \
special-cased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I'm open for debate - any other opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a deprecation for these methods in favor of the non-dot functions, to help anyone who is expecting them to produce a range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what do we want here? We can't deprecate methods that don't exist on v0.6, but I could just remove these definitions. But as a user I'd be a bit confused why I need .+
but can't use .*
, however the dot-call lowering is pretty complex also. (The ranges stuff interacting with broadcast
was clearly the worst part of doing this PR; it's kind of awkward to preserve Range
ness now).
2034c1f
to
6fb1248
Compare
Triage is for this so could you finish it @andyferris? |
Bumbity, bumbity. |
Yes, sorry, real world pressures ATM... I will be finishing this when I can. |
Is there anything else than rebasing to be done? |
@fredrikekre I was worried I messed up a force push at some point and lost some final fixes I had done... and I haven't gotten back to this to fix it up. |
Let us know if there's anything else we can do to help! |
Is array conceptually the same thing as matrix in above context and in Julia? I have this question because array in general could represent more concept than matrix from linear algebra. For example, it's common to use a 2D array as a pixel container for a 2D bitmap image. Then "array + scalar" means operation on each pixel in the container, just nothing to do with the matrix math. Edit: For arrays that are higher than 3D(eg. m x n x k), matrix multiplication has no definition and not supported. Also there is no "one"(either Identity or UniformScaling) defined for these arrays. Sorry to ask this question here if it is not appropriate for a non-contributor. |
@morningkyle Julia has a standard and dedicated way of operating on each element of a container, such as the pixels of an image, using We've generally heavily overloaded arrays with linear algebra operations like |
Fixes deprecation due to JuliaLang/julia#22932.
@andyferris Yes, deprecating "matrix + scalar" makes sense to me. I am not sure if deprecating "array + scalar" also drops other conveniences. Let me give one more code example below: Edit myself: We should use the dot-call syntax f.(a) to accept the array input. There is no inconvenience if the dot-call syntax is well accepted in above example. |
You're right - exactly. As a general rule of thumb, genericism can only go so far. One thing we've tended to do in Julia is separate out functions which act on "containers of objects" from functions acting on "objects" (scalars, or whatever) themselves. Think
I feel it would be really confusing if this worked differently for 2D arrays, than all the other dimensions. |
It depends on what you mean. |
Array: a continuous block of data of the same element type. With element type be scalar, array supports arbitrary arithmetic on each single element, thus a very general and fundamental data structure. Matrix: linear algebra concept, an m × n array of scalars from a given field F. So matrix is just a subset form of array and supports restricted mathematical operations. That's the main point I think deprecating "matrix + scalar" makes sense. Especially, arrays that are equal or higher than 3D(m x n x k) are not like matrix at all(eg. matrix like multiplication or * operator has no definition for 3D array). Now let's try to define "array + scalar". Now come back to these points:
If x is a matrix, then we could either make "2x + 1" an invalid operation or make it a convenient form for of 2x + I. The current choice is the former. And it makes sense. I have no problem with this choice. But if x is an array, based on above "array + scalar" definition, 2x + 1 is well defined and convenient. 2x .+ 1 is also well defined and not inconvenient. But why should we stop people using the 2x + 1 format? If x is an array, we should not assume it as a matrix unless in a predefined context that we know we are working with linear algebra. Without such specific context, it's easy and natural to just think x as a block of data. Because array is a common and widely used concept in other programing languages(C/C++/C#/Java/Python) before Julia. Taking "array + scalar" as a convenient form of block modification, then keeping the form is not confusing and makes sense to me. Python array(from numpy) supports "array + scalar" and Matlab matrix supports "matrix + scalar". I don't argue that Julia should do the same, just meant I did not invent the "array + scalar" concept. |
I think that the problem is that matrix is a type of array, so defining array+scaler in a way that doesn't make sense for matrix will lead to a definition of matrix+ array that doesn't make sense |
@morningkyle The reason it is considered confusing to support vector + scalar is that this is not consistent with how other operations work. It is better for a user migrating from Python or Matlab to realize that things like Because as you mention the convention |
@morningkyle I feel that you are using definitions here which aren't true in Julia. In particular, given:
I think that you are taking our array to be similar to those in MATLAB, numpy, etc, where |
It's true in following sense(code). Julia array itself is still very similar to those in other languages. See the first statement in https://docs.julialang.org/en/stable/manual/arrays/. It just that Julia built-in operators/functions do not simulate other languages(eg. Python and Matlab). Anyway, I got your point(built-in arithmetic functions generally do not accept array input without dot-call syntax) and thanks very much!
|
It's not mentioned here, but the reason we need to get rid of
julia> (zeros(3,3) + 1) + I
3×3 Array{Float64,2}:
2.0 1.0 1.0
1.0 2.0 1.0
1.0 1.0 2.0
julia> zeros(3,3) + (1 + I)
3×3 Array{Float64,2}:
2.0 2.0 2.0
2.0 2.0 2.0
2.0 2.0 2.0 If the first |
Similarly, it would great if the distributive law worked, for example |
Both "matrix + scalar" and "array + I(identity)" are odd operations without brain type conversion first. Now your example mixed the two: BTW, zeros(m, n) returns an literal array for all documentations from Julia/Python/Matlab. As a conclusion to myself after these discussion, it is subtle to replace "array + scalar" with "array .+ scalar", but I am getting used to it. |
The point is that when writing generic code, you don't know what specific implementations of |
This is a good point. What I think is that your example is confusing. It does not support your (this agreed) point very well, then does not support the logic to "get rid of A + 1" very well.
(1) If I is an identity matrix here, then associativity is satisfied.
|
Yes, that's why we're changing it. |
The arguments behind this are very convincing, and I'm a supporter. But I just noticed a little issue that would be hilarious if it weren't so...well, you'll see. Watch the julia version numbers and return types: | | |_| | | | (_| | | Version 0.5.2 (2017-05-06 16:34 UTC)
_/ |\__'_|_|_|\__'_| |
|__/ | x86_64-linux-gnu
julia> (1:5) + 1
2:6
julia> (1:5) .+ 1
2:6
julia> broadcast(+, 1:5, 1)
5-element Array{Int64,1}:
2
3
4
5
6 \ | | |_| | | | (_| | | Version 0.6.1-pre.0 (2017-06-19 13:06 UTC)
_/ |\__'_|_|_|\__'_| | Commit dcf39a1 (92 days old release-0.6)
|__/ | x86_64-linux-gnu
julia> (1:5) + 1
2:6
julia> (1:5) .+ 1
5-element Array{Int64,1}:
2
3
4
5
6
julia> broadcast(+, 1:5, 1)
5-element Array{Int64,1}:
2
3
4
5
6 | | |_| | | | (_| | | Version 0.7.0-DEV.1846 (2017-09-19 12:18 UTC)
_/ |\__'_|_|_|\__'_| | Commit 77cbd18* (0 days old master)
|__/ | x86_64-linux-gnu
julia> (1:5) + 1
WARNING: a::AbstractArray + b::Number is deprecated, use broadcast(+, a, b) instead.
Stacktrace:
[1] depwarn(::String, ::Symbol) at ./deprecated.jl:68
[2] +(::UnitRange{Int64}, ::Int64) at ./deprecated.jl:56
[3] eval(::Module, ::Expr) at ./repl/REPL.jl:3
[4] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
[5] macro expansion at /home/tim/.julia/v0.7/Revise/src/Revise.jl:748 [inlined]
[6] (::getfield(Revise, Symbol("##11#12")){Base.REPL.REPLBackend})() at ./event.jl:96
while loading no file, in expression starting on line 0
2:6
julia> (1:5) .+ 1
5-element Array{Int64,1}:
2
3
4
5
6
julia> broadcast(+, 1:5, 1)
2:6 Ha ha. Ugh. |
Haha. Yeah... sorry for contributing to that :) Making.broadcast fusion more "transparent" to code may solve this neatly. |
I don't think that's any sort of show-stopper, I think that's really just a series of accidental behaviors flip-flopping. Correct behaviors IMO: (1:5) + 1 => error
(1:5) .+ 1 => 2:5
broadcast(+, 1:5, 1) => 2:5 Fortunately, we're quite close to that already and just need to fix broadcasting over ranges. Even if we don't it's not really all that significant since |
It's definitely not a show-stopper. It does make it very likely that some packages (especially those that define new types of EDIT: Probably best would be to have a |
This removed |
Hmm. Maybe I thought that method was a bug, not a feature? A deprecation seems fine. |
It was a bug. It must have been hiding the range code when the method for vectors was removed. Still fine to deprecate, though. |
There has been much discussion around this lately. This PR deprecates
array + scalar
(and related) in favor ofarray .+ scalar
for v0.7 (with the possibility of introducing behavior such thatmatrix + scalar == matrix + scalar*I
in v1.0 in a separate PR).The arguments for changing the behavior of
array + scalar
from elementwise are:Number
/AbstractArray
methods deprecated in this PR, all the other methods of+
,-
,*
,/
involvingAbstractArray
are motivated by and consistent with linear algebra. For instancevector + vector
is defined becausevector
belongs to a vector space (not becausevector .+ vector
is inconvenient) and a such, the entire container adds (not just the elements). However, the current element-wise definition seems to be incorrect for linear algebra. For instance, if I write a polynomial function such asf(a,b,c,x) = a*x^2 + b*x + c
, I expect it to work for anyx
that belongs to a field/ring/whatever. However, this definition is broken ifx
is a square matrix - you expect behavior likef(a,b,c,x) = a*x^2 + b*x + c*I
but currently thec
is added to all parts of the matrix, not just the diagonal part. It is therefore difficult to write generic code that works for a wide range of types ofx
(though I would notea*x*x + b*x + c*one(x)
currently works forx::Number
andx::AbstractMatrix
, though the extraone
seems unnatural).one
that you might expect, wherebyarray + scalar != array + scalar*one(array)
sin(array)
no longer works; rathersin.(array)
is necessary). In this situation, I would actually argue thatarray .+ scalar
for elementwise addition is less confusing... it's less of a special case.The primary arguments against (that I am aware of) are:
array + scalar
behavior is convenient (though.+
is hardly much harder to type that+
).