promote_op() fixes #16995

nalimilan · 2016-06-17T18:43:09Z

I've added systematic tests for promote_op and operators on numbers, and they uncovered a few corner cases which either didn't work at all, were type-unstable, or for which promote_op did not match the actual type. See commit messages for details.

The first commit comes from #16996 and is needed for all tests to pass, so I moved it here.

Cf. #16988. Fixes #4883.

nalimilan · 2016-06-19T17:43:26Z

@JeffBezanson I've found one case where the promote_op fallback to promote_type(S, T) was used: for operations on Complex. This implied that *(::Matrix{Complex}, ::Matrix{Complex}) returned a Matrix{Complex}, while the rule based on one would return a Matrix{Complex{Int}}.

While more consistent, the latter has the drawback that * no longer works when the matrix contains e.g. Complex{Float64} (which is why I changed a test in linalg/dense.jl). For example:

julia> dim=2
2

julia> S=zeros(Complex,dim,dim)
2×2 Array{Complex,2}:
 0+0im  0+0im
 0+0im  0+0im

julia> T=zeros(Complex,dim,dim)
2×2 Array{Complex,2}:
 0+0im  0+0im
 0+0im  0+0im

julia> T[:] = 1
1

julia> z = 2.5 + 1.5im
2.5 + 1.5im

julia> S[1] = z
2.5 + 1.5im

julia> S*T
ERROR: InexactError()
 in convert(::Type{Int64}, ::Float64) at ./int.jl:239
 in matmul2x2!(::Array{Complex{Int64},2}, ::Char, ::Char, ::Array{Complex,2}, ::Array{Complex,2}) at ./linalg/matmul.jl:662
 in generic_matmatmul!(::Array{Complex{Int64},2}, ::Char, ::Char, ::Array{Complex,2}, ::Array{Complex,2}) at ./linalg/matmul.jl:441
 in *(::Array{Complex,2}, ::Array{Complex,2}) at ./linalg/matmul.jl:129
 in eval(::Module, ::Any) at ./boot.jl:231
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

This problem has been discussed at #4796.

OTOH, the same problem currently exists with reals:

julia> Real[1 2] * Real[1.5; 2.]
ERROR: InexactError()
 in convert(::Type{Int64}, ::Float64) at ./int.jl:239
 in generic_matvecmul!(::Array{Int64,1}, ::Char, ::Array{Real,2}, ::Array{Real,1}) at ./linalg/matmul.jl:416
 in *(::Array{Real,2}, ::Array{Real,1}) at ./linalg/matmul.jl:84
 in eval(::Module, ::Any) at ./boot.jl:231
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

Instead of the current hack, one solution would be to change one for Real (or even Number?) to return 1.0::Float64 by default, instead of 1::Int. Indeed floats are a much better type for representing any real than integers. This is what #6183 did, but was reverted by JuliaLang/LinearAlgebra.jl#102 since it caused issues. On another open issue (#4808), the consensus seems to be that we need to make one an error on abstract types. Should we also make * an error?

nalimilan · 2016-06-19T21:37:34Z

test/broadcast.jl

@@ -165,9 +165,9 @@ m = [1:2;]'
 @test @inferred([0,1.2].+reshape([0,-2],1,1,2)) == reshape([0 -2; 1.2 -0.8],2,1,2)
 rt = Base.return_types(.+, Tuple{Array{Float64, 3}, Array{Int, 1}})
 @test length(rt) == 1 && rt[1] == Array{Float64, 3}
-rt = Base.return_types(broadcast, Tuple{Function, Array{Float64, 3}, Array{Int, 1}})
+rt = Base.return_types(broadcast, Tuple{typeof(+), Array{Float64, 3}, Array{Int, 1}})


This is another place where the return type is assumed to be equal to promote_type on the inputs. It breaks with the promote_op definition based on typeof(op(one(R), one(S))), since the exact function needs to be specified to call it.

nalimilan · 2016-06-20T10:34:35Z

I think I've found a better solution to the abstract types problem from my last comment: in promote_op, check whether one of the input types is a supertype of the operation's return type, and if so return the most general type. This is a simple rule, and it even fixes Real[1 2] * Real[1.5; 2.]. I've updated the PR with this new approach.

nalimilan · 2016-06-20T17:19:24Z

base/irrationals.jl

@@ -10,6 +10,10 @@ promote_rule{s}(::Type{Irrational{s}}, ::Type{Float32}) = Float32
 promote_rule{s,t}(::Type{Irrational{s}}, ::Type{Irrational{t}}) = Float64
 promote_rule{s,T<:Number}(::Type{Irrational{s}}, ::Type{T}) = promote_type(Float64,T)

+promote_op{S<:Irrational,T<:Irrational}(::Any, ::Type{S}, ::Type{T}) = Float64
+promote_op{S<:Irrational,T<:Number}(::Any, ::Type{S}, ::Type{T}) = promote_type(S, T)


Irrational looks like a valid case where promote_op should rely on promote_type: irrationals by definition cannot be represented ~~correctly~~ perfectly by any other type, and no other type is closed under operations with irrationals. Therefore, the most appropriate type for the result is the same for all operations, and is obtained using promotion rules.

the most appropriate type for the result is the same for all operations

Only for all operations of a particular, numerical variety. Not for things like converting to string, etc.

Of course. One issue is that the scope of promote_op isn't clearly defined: is "op" only for standard operators? For any operation? That's a general fallback anyway.

That's true, but broadcast uses it for everything. Since broadcast doesn't know when promote_op is the right thing to use, these fallbacks continue to get in the way.

What would broadcast use if we removed the fallbacks? What do you think we should do?

@JeffBezanson commented on #16986 that we might want to have the fallbacks as Union{} and use an algorithm similar to the one map uses by dispatching on Union{}.

If the fallbacks returned Union{} or perhaps Any, we could dispatch to a version of broadcast that uses the map algorithm in that case.

But which fallbacks? Should we keep the Numbers methods? Or should we restrict them to the combination of numbers and known operators?

I don't think it ever makes sense to have a definition of promote_op that ignores the function argument --- except one fallback that returns a sentinel value like Any.

I general I fully agree. But for the special case of numbers, the use of one means that the function argument is taken into account.

Anyway, I only care to the extent that we need a way to get the return type to implement operators on Nullable (which are the origin of this investigation).

nalimilan · 2016-06-27T08:16:18Z

@JeffBezanson Could you tell me which part of this PR (and of additional fixes at #17114) you would accept, and what we should fix? This is blocking operators support for Nullable (JuliaStats/NullableArrays.jl#119), which in turns blocks progress on porting DataFrames to Nullable.

JeffBezanson · 2016-06-28T20:18:11Z

I would prefer to remove the definitions that ignore op. But if that really cannot be done now, you can just go ahead with this PR (tests seem to have timed out for some reason).

pabloferz · 2016-06-28T20:40:59Z

I think that in #17114 I managed to leave only promote_ops that take op into account and anything else will be Any. Unless of course I missed something and did something wrong.

nalimilan · 2016-06-28T21:35:26Z

I don't think we can get rid of the method for numbers which is based on one, but no other method applying to any op is added.

I've just picked further changes by @pabloferz, let's see the result.

pabloferz · 2016-06-28T21:59:04Z

base/float.jl

 for f in (ceil, floor, round, trunc)
-    @eval promote_op{Op<:$(typeof(f)),T}(::Op, ::Type{T}, ::Any) = T
+    @eval promote_op{T}(::typeof($f), ::Type{T}, ::Any) = T


There's going to be a lot of ambiguities with these changes that are going to be too cumbersome to solve otherwise. That's why I had this, but maybe there's another (better) way to handle those.

Why? It shouldn't make any difference from the previous syntax.

For the same reason you added versions for <, >, ==, etc. parametrised with Number. You'll need versions parametrised with Number and Nullable for convert, ceil, parse, etc.

And the definitions I had would make a difference since you'd have methods with a parameter for the first argument that are more specific than the definitions for Number and Nullable.

Have you actually tested it? In my tests, the Op<: form has exactly the same ambiguities.

JeffBezanson · 2016-06-29T04:04:35Z

Yes, the definitions that do op(one(T)) are fine.

Don't worry too much about this --- #16622 adds a fairly useful return_type function, and #17172 adds a fallback for broadcast that will give a good result when promote_op returns Any.

nalimilan · 2016-06-29T15:43:40Z

@JeffBezanson Does that mean we don't need the methods for convert, parse round, etc.? They create a lot of ambiguities with the Number methods.

JeffBezanson · 2016-06-29T22:03:58Z

The promote_op method calling promote_type could be restored for Union{typeof(+), typeof(-), ...} as the type of op.

nalimilan · 2016-06-30T10:09:42Z

The promote_op method calling promote_type could be restored for Union{typeof(+), typeof(-), ...} as the type of op.

I don't really like that solution either, since that rule isn't correct in many cases: -(::Date, ::Date) is an example for which we have a custom promote_op rule in Base. I'd rather return Any and have an algorithm find the most appropriate type (maybe using return type declarations? that would be a nicer API than promote_op).

So, as regards the resolve failure in Base, I've just added promote_op methods for + and -, which are the only supported operations. We can easily revise this later when the whole system will have been improved.

pabloferz · 2016-06-30T16:30:57Z

base/number.jl

+    # preserve the most general (abstract) type when possible
+    return isleaftype(T) ? S : typejoin(S, T)
+end
+function promote_op{R<:Number,S<:Number}(op, ::Type{R}, ::Type{S})


Maybe we can write

x = R <: Irrational ? 1.0 : one(R) y = S <: Irrational ? 1.0 : one(S) T = typeof(op(x, y))

and remove the ones for Irrational above? (and restore the comparison ones)

Yeah, I thought about that possibility too. In general though, I think it's considered more Julian to dispatch on types.

That solution would be quite appealing to avoid ambiguities with the methods for comparison operators, but we still need to take care of ambiguities with the Nullable method. So I'd rather get rid of the comparison operators method altogether. That fits with @JeffBezanson's request. (But currently it fails the tests, not sure why yet.)

I now dispatching in types is more Julian. The problem here, if I understand Jeff right, is that any method that ignores op, that is promote_op(op, ...) should preferably return Any, and that we should have as fewer such methods as possible (but also the as fewer as possible of the ones that take op into account). But maybe we should still follow the Julian way everywhere.

The methods that have Number types as arguments (other than op) are a special case since we use them in other places besides broadcast. The methods with Nullable types are probably also important on its own.

~~The comparison methods are failing because there is no isless for Complex and other numerical types.~~ I'm not sure why the comparison methods can't be inferred with this definition, that's why I had the comparison ones.

I'm not sure why the comparison methods can't be inferred with this definition, that's why I had the comparison ones.

I suspect there's a type-instability somewhere. These tests already caught one in a corner case.

Found a minimum reproducer: Base.Test.@inferred <(one(Rational{Int32}), one(Float64)) (only happens on 32-bit)

pabloferz · 2016-07-01T08:24:04Z

base/irrationals.jl

@@ -10,6 +10,13 @@ promote_rule{s}(::Type{Irrational{s}}, ::Type{Float32}) = Float32
 promote_rule{s,t}(::Type{Irrational{s}}, ::Type{Irrational{t}}) = Float64
 promote_rule{s,T<:Number}(::Type{Irrational{s}}, ::Type{T}) = promote_type(Float64,T)

+promote_op{S<:Irrational,T<:Irrational}(::Any, ::Type{S}, ::Type{T}) =


This is missing op as first argument

Good catch. Will fix, but the CI hang doesn't seem related...

pabloferz · 2016-07-01T12:01:31Z

base/intfuncs.jl

@@ -198,7 +198,7 @@ end
 function ndigits0z(n::Unsigned, b::Int)
    d = 0
    if b < 0
-        d = ndigits0znb(signed(n), b)
+        d = ndigits0znb(n, b)
    else


Seems that CI has been hanging all along because of this (see https://github.com/JuliaLang/julia/blob/master/test/intfuncs.jl#L78)

Thank you so much for catching it! How did you found out that was the culprit?

I looked in test/choosetests.jl to see if there was some of the tests missing in the log.

I had tried that too, but I guess it didn't do it systematically enough. Adding a timeout on individual tests would indeed make this much clearer...

nalimilan · 2016-07-01T13:16:26Z

add some @inferred tests for this?

The new tests cover it already, that's why they were failing.

pabloferz · 2016-07-01T14:01:33Z

AV failure seems to be #16091

nalimilan · 2016-07-01T14:15:21Z

Yes, I've restarted it hoping we'll be more lucky this time.

Good news: the Travis CI build passed! Is there anything left to change? I'd be in favor of merging if it's globally OK, and possibly tweak details in subsequent PRs.

JeffBezanson · 2016-07-01T17:06:15Z

Thanks for persisting on this! Could you squash some of the commits? In particular would be good to keep the ones that fix general issues separate, and squash together the promote_op changes.

tkelman · 2016-07-01T23:59:35Z

base/intfuncs.jl

@@ -198,7 +198,7 @@ end
 function ndigits0z(n::Unsigned, b::Int)
    d = 0
    if b < 0
-        d = ndigits0znb(n, b)
+        d = ndigits0znb(signed(n), b)


is this still right if it wraps to negative?

This change actually cancels the one I incorrectly made in a previous commit. So after squashing nothing changes here.

I guess consider that a question w.r.t. master then?

I think so, since after removing signed the unsigned wrapping triggered a failure. So I guess this means with signed, the wrapping is correct (since the test works). But you'll have to find somebody who knows this code to get an actual answer. :-)

Need at least 64-bit to store the result of Float64 decomposition. (A very good use case for return type declarations.)

These only affected particular type combinations, with either type instabilities which affected inference of return type or plain failures. Tests are included in the next commit.

Use common promote_op() based on one() for all Number types: this fixes promote_op(+, ::Bool), which returned Int, and promote_op(==, ::Complex, ::Complex), which returned Complex{Bool}. Add fallback definitions returning Any so that broadcast() computes the appropriate return type, instead of using promote_type() which is often wrong. This fixes broadcast() when the return type is different from the input type. Add systematic tests for operators and their return types.

nalimilan · 2016-07-02T11:58:06Z

Squashed and rebased, tests still pass!

nalimilan · 2016-07-02T12:00:19Z

base/promotion.jl

 promote_op{T}(::Type{T}, ::Any) = (@_pure_meta; T)
-promote_op{R,S}(::Any, ::Type{R}, ::Type{S}) = (@_pure_meta; promote_type(R, S))
-promote_op(op, T, S, U, V...) = (@_pure_meta; promote_op(op, T, promote_op(op, S, U, V...)))


I've removed this method when squashing since it doesn't make sense to use the output type as an input type now that the fallback to promote_type has been removed. I've replaced it with promote_op(::Any, ::Any, ::Any...) above.

Makes sense 👍. ~~Isn't this just missing promote_op(::Any, ::Any) = Any?~~ NVM I didn't see the splatting above.

No, because ::Any... can also match no argument. This is an annoying detail with current varargs.

stevengj · 2016-07-03T12:34:28Z

base/complex.jl

@@ -461,7 +461,7 @@ function ^{T<:AbstractFloat}(z::Complex{T}, p::Complex{T})
    if p==2 #square
        zr, zi = reim(z)
        x = (zr-zi)*(zr+zi)
-        y = 2zr*zi
+        y = T(2)*zr*zi


Why is this necessary? It seems like this will be slower e.g. for Complex{BigFloat}

Because of the type instability it creates for Float16:

julia> typeof(2*Float16(1.0)) Float32

Maybe better write it as T(2*zr*zi)?

(I wonder whether that promotion exception for Float16 is really a good idea...)

Yes, I think T(2*zr*zi) seems like a better way to do it.

See #17259.

nalimilan mentioned this pull request Jun 17, 2016

RFC: Implement operators on Nullable with lifting semantics #16988

Closed

nalimilan force-pushed the nl/promote_op branch from fedbc97 to bab0fbb Compare June 18, 2016 11:44

nalimilan changed the title ~~Add unary promote_op() for Bool~~ promote_op() fixes Jun 18, 2016

nalimilan force-pushed the nl/promote_op branch from bab0fbb to 73240a8 Compare June 18, 2016 12:11

nalimilan mentioned this pull request Jun 18, 2016

Fix bitshift operators on Bool #16996

Merged

nalimilan force-pushed the nl/promote_op branch from 73240a8 to 70ef34d Compare June 18, 2016 18:16

This was referenced Jun 18, 2016

Improve operators JuliaStats/NullableArrays.jl#119

Merged

Generalize broadcast to handle tuples and scalars #16986

Merged

nalimilan force-pushed the nl/promote_op branch from 70ef34d to 6c0695e Compare June 19, 2016 17:43

nalimilan force-pushed the nl/promote_op branch from 6c0695e to c3cca29 Compare June 19, 2016 21:34

nalimilan reviewed Jun 19, 2016
View reviewed changes

nalimilan changed the title ~~promote_op() fixes~~ WIP: promote_op() fixes Jun 19, 2016

nalimilan force-pushed the nl/promote_op branch from c3cca29 to d754623 Compare June 20, 2016 09:53

nalimilan force-pushed the nl/promote_op branch 4 times, most recently from 7da178c to 4e1ce13 Compare June 20, 2016 17:16

nalimilan reviewed Jun 20, 2016
View reviewed changes

nalimilan force-pushed the nl/promote_op branch from 4e1ce13 to 21bc568 Compare June 28, 2016 09:28

nalimilan mentioned this pull request Jun 28, 2016

More promote_op fixes #17114

Closed

nalimilan force-pushed the nl/promote_op branch from 21bc568 to 9df4f1a Compare June 28, 2016 21:01

pabloferz reviewed Jun 28, 2016
View reviewed changes

nalimilan force-pushed the nl/promote_op branch 2 times, most recently from 319dfd7 to ac429cb Compare June 30, 2016 15:14

pabloferz reviewed Jun 30, 2016
View reviewed changes

pabloferz mentioned this pull request Jun 30, 2016

Make rational vs float comparisons inferrable #17221

Closed

nalimilan force-pushed the nl/promote_op branch from dbc3e30 to c20d8d4 Compare July 1, 2016 07:26

pabloferz reviewed Jul 1, 2016
View reviewed changes

nalimilan force-pushed the nl/promote_op branch 2 times, most recently from 69a31e0 to 7d70ae1 Compare July 1, 2016 11:53

pabloferz reviewed Jul 1, 2016
View reviewed changes

nalimilan force-pushed the nl/promote_op branch 2 times, most recently from 35cf7de to 7a0d383 Compare July 1, 2016 13:03

nalimilan changed the title ~~WIP: promote_op() fixes~~ promote_op() fixes Jul 1, 2016

tkelman reviewed Jul 1, 2016
View reviewed changes

nalimilan force-pushed the nl/promote_op branch from 7a0d383 to 4807c14 Compare July 2, 2016 10:54

nalimilan added 3 commits July 2, 2016 12:55

Fix type instability in decompose(::Float64) on 32-bit

f3eabc3

Need at least 64-bit to store the result of Float64 decomposition. (A very good use case for return type declarations.)

Fix various corner cases with operators on Numbers

732828c

These only affected particular type combinations, with either type instabilities which affected inference of return type or plain failures. Tests are included in the next commit.

nalimilan force-pushed the nl/promote_op branch from 4807c14 to 17dddc0 Compare July 2, 2016 10:57

nalimilan reviewed Jul 2, 2016
View reviewed changes

JeffBezanson merged commit a88a1b9 into master Jul 2, 2016

tkelman deleted the nl/promote_op branch July 2, 2016 23:28

stevengj reviewed Jul 3, 2016
View reviewed changes

martinholters mentioned this pull request Jul 4, 2016

0.5 Adding matrices of special type incorrectly promoted to Matrix{Any} #17254

Closed

promote_op() fixes #16995

promote_op() fixes #16995

Conversation

nalimilan commented Jun 17, 2016 • edited Loading

nalimilan commented Jun 19, 2016 • edited Loading

Choose a reason for hiding this comment

nalimilan commented Jun 20, 2016

nalimilan Jun 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Jun 27, 2016

JeffBezanson commented Jun 28, 2016

pabloferz commented Jun 28, 2016

nalimilan commented Jun 28, 2016

pabloferz Jun 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz Jun 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jun 29, 2016

nalimilan commented Jun 29, 2016

JeffBezanson commented Jun 29, 2016

nalimilan commented Jun 30, 2016

pabloferz Jun 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz Jun 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz Jul 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Jul 1, 2016 • edited by yuyichao Loading

pabloferz commented Jul 1, 2016

nalimilan commented Jul 1, 2016

JeffBezanson commented Jul 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Jul 2, 2016

Choose a reason for hiding this comment

pabloferz Jul 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Jun 17, 2016 •

edited

Loading

nalimilan commented Jun 19, 2016 •

edited

Loading

nalimilan Jun 20, 2016 •

edited

Loading

pabloferz Jun 28, 2016 •

edited

Loading

pabloferz Jun 28, 2016 •

edited

Loading

pabloferz Jun 30, 2016 •

edited

Loading

pabloferz Jun 30, 2016 •

edited

Loading

pabloferz Jul 1, 2016 •

edited

Loading

nalimilan commented Jul 1, 2016 •

edited by yuyichao

Loading

pabloferz Jul 2, 2016 •

edited

Loading