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

0.6 undocumented incompatibility/change? #20946

Closed
sbromberger opened this issue Mar 8, 2017 · 32 comments
Closed

0.6 undocumented incompatibility/change? #20946

sbromberger opened this issue Mar 8, 2017 · 32 comments

Comments

@sbromberger
Copy link
Contributor

sbromberger commented Mar 8, 2017

per https://discourse.julialang.org/t/is-this-expected-behavior-0-5-to-0-6-breakage/2534/7

In 0.5:

julia> a = Vector{UInt32}([0x01, 0x02, 0x03])
3-element Array{UInt32,1}:
 0x00000001
 0x00000002
 0x00000003

julia> a + 1
3-element Array{UInt32,1}:
 0x00000002
 0x00000003
 0x00000004

In 0.6:

julia> a = Vector{UInt32}([0x01, 0x02, 0x03])
3-element Array{UInt32,1}:
 0x00000001
 0x00000002
 0x00000003

julia> a + 1
3-element Array{Int64,1}:
 2
 3
 4

(Is there a reason this change was made? FWIW, I prefer the old behavior. Changing the type out from under me broke a bunch of things, most significantly some code for generating random graphs:

Base.dSFMT.dsfmt_gv_init_by_array(MersenneTwister(seed).seed+1)

Unless you know that MersenneTwister's seed is a Vector{UInt32}, you wouldn't know that adding 1 to it would cause a type change and subsequent failure. Ironically, if it were a Vector{Char}, everything would have just worked:

julia> a = Vector{Char}(['a','b','c'])
3-element Array{Char,1}:
 'a'
 'b'
 'c'

julia> a + 1
3-element Array{Char,1}:
 'b'
 'c'
 'd'

I couldn't find anything in NEWS.md on this change, so at a minimum, I think a note would be warranted if this new behavior is intended.

@KristofferC
Copy link
Sponsor Member

#19692.

There is a NEWS entry about it.

@sbromberger
Copy link
Contributor Author

sbromberger commented Mar 8, 2017

You're referring to this entry?

The array-scalar methods of /, , *, +, and - now follow broadcast promotion rules. (Likewise for the now-deprecated array-scalar methods of div, mod, rem, &, |, and xor; see "Deprecated or removed" below.) (#19692).

With all respect, that's probably the least intuitive explanation for what's happening here that I can imagine.

@KristofferC
Copy link
Sponsor Member

Perhaps make a PR with something along the line "Consequently, Float32[1.0, 2.0] * 3.0 results in a Vector{Float64}".

@sbromberger
Copy link
Contributor Author

sbromberger commented Mar 8, 2017

Just to clarify, Vector{UInt32}([0x1,0x2]) + 1 will only yield a Vector{Int64} on 64-bit systems, right? If so, that's even more confusing.

@KristofferC
Copy link
Sponsor Member

Why is that confusing? It works just as for scalar op scalar

@StefanKarpinski
Copy link
Sponsor Member

The fact that it works the same as the scalar ops is good. There's a separate issue that perhaps the scalar ops should have different behaviors than they do, but that's a different issue.

@kmsquire
Copy link
Member

kmsquire commented Mar 8, 2017

I think I'm in agreement with @sbromberger here. This change does make it harder to write code that works the same on both 32 and 64-bit systems.

I've been playing around with this, and the results don't really mesh with my intuition. Some interesting examples--can you predict the output types on 32-bit and 64-bit systems?:

f(x) = (x.+=1; x)
g(x) = x.+1

x = UInt64[1,2,3]
y = UInt32[1,2,3]
z = Int32[1,2,3]

f(x)
f(y)
f(z)

g(x)
g(y)
g(z)

@KristofferC
Copy link
Sponsor Member

This is confusing behavior to me:

julia> function f(x)
           println("eltype in ", eltype(x))
           v = x + 1
           println("eltype out ", eltype(v))
           return v
       end;

julia> f(Int32[1,2,3]);
eltype in Int32
eltype out Int32

julia> f(Int32(1));
eltype in Int32
eltype out Int64

@kmsquire
Copy link
Member

kmsquire commented Mar 8, 2017

@KristofferC, that make sense. And from my perspective, it is also confusing to me when my array variable type changes from under me:

julia> function f(x)
           println("type in ", typeof(x))
           x += 1
           println("type out ", typeof(x))
           return x
       end;

julia> f(UInt32[1,2,3]);
type in Array{UInt32,1}
type out Array{Int64,1}

@yuyichao
Copy link
Contributor

yuyichao commented Mar 8, 2017

So do you expect [1] + 1.5 to throw an error?

@KristofferC
Copy link
Sponsor Member

Your array type didn't change. You created a brand new array.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Mar 8, 2017

It it kind of annoying that x ⨳= y can change the type of x so easily. I wonder if it would make sense to lower it to x = oftype(x, x ⨳ y) instead of just x = x ⨳ y. One would probably be hard pressed to find a case where changing the type of x in these kinds of expressions isn't a bug.

@joa-quim
Copy link

joa-quim commented Mar 8, 2017

I'm also of the opinion that this behavior of integers is unfortunate but it all roots on the, also unfortunate IMO, decision that integers default to int64.

@pabloferz
Copy link
Contributor

pabloferz commented Mar 8, 2017

X-ref: #19669

If you need to preserve the type of your container you now have many options:

  • Modify your array in place. E.g. Float32[1.0] .+= 1.0
  • In some cases you could use an scalar such that the return type is the same as the array type. Float32[1.0] + 1.f0
  • Use a comprehension. T=eltype(A) and T[a ⨳ s for a in A] or [T(a ⨳ s) for a in A]
  • Take advantage of dot fusion. T.(s .⨳ A)

@StefanKarpinski
Copy link
Sponsor Member

I'm also of the opinion that this behavior of integers is unfortunate but it all roots on the, also unfortunate IMO, decision that integers default to int64.

What would you propose that integers default to?

@StefanKarpinski
Copy link
Sponsor Member

@pabloferz: I was actually think more in terms of scalar operations, where I often have to take pains to get arguments to the same type, e.g. when writing things like x += 1.

@joa-quim
Copy link

joa-quim commented Mar 8, 2017

What would you propose that integers default to?

I would have preferred that they defaulted to int32, both in 32 and 64 bits systems

@pabloferz
Copy link
Contributor

@StefanKarpinski I was referring to the OP for the cases where you have to work with arrays.

For scalars maybe #19992 could be relevant. There it is proposed to make .= work with immutables. So maybe we could have .⨳= to preserve the type on the LHS when immutable?

@sbromberger
Copy link
Contributor Author

So do you expect [1] + 1.5 to throw an error?

@yuyichao no, in that case I expect to have a Vector{Float64}. But Floats are a fundamentally different type than Ints; I guess my preference would be this:

if the resulting value can be represented as the same type as the first operand
    then it should take the type of the first operand
else if the resulting value can be represented as the same type as the second operand
    then it should take the type of the second operand
else if the resulting value can be promoted
    then it should be promoted
else throw an error

@kmsquire
Copy link
Member

kmsquire commented Mar 8, 2017

@KristofferC said:

Your array type didn't change. You created a brand new array.

Sorry, you're right. What I meant (and should have said) was that my variable type changed.

@GunnarFarneback
Copy link
Contributor

Perhaps make a PR with something along the line "Consequently, Float32[1.0, 2.0] * 3.0 results in a Vector{Float64}".

Haven't we been here before, e.g. #1641? This feels a lot like a regression.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Mar 9, 2017

It is not a "regression" because it was an intentional change. Writing generic code you should not really care if your function get passed an array or a number and you want to apply an operation to all the elements. It is inconsistent to me to return different element types depending on if 1.0 or [1.0] got passed.

The comment you linked are from five years ago. A lot of things have changed since then.

@GunnarFarneback
Copy link
Contributor

It's probably not that constructive for two Swedes to discuss the meaning of English words but I'm fairly certain you can regress even if you do it by intention. In this case it's a regression to the state of Julia before #1641, which was also an intentional change.

(Personally I'd solve both the problem of blowing up arrays and consistency between arrays and scalars by promoting Float32, Float64 to Float32, but that has been hard to sell.)

@sbromberger
Copy link
Contributor Author

I'd solve both the problem of blowing up arrays and consistency between arrays and scalars by promoting Float32, Float64 to Float32

Yes. "principle of least surprise", and all.

@sunoru
Copy link
Contributor

sunoru commented Mar 13, 2017

@KristofferC What's your Julia version? I got this with your code:

julia> function f(x)
           println("eltype in ", eltype(x))
           v = x + 1
           println("eltype out ", eltype(v))
           return v
       end;

julia> f(Int32[1,2,3]);
eltype in Int32
eltype out **Int64**

julia> f(Int32(1));
eltype in Int32
eltype out Int64

And BTW this is also annoying to me:

julia> f(Int64(1));
eltype in Int64
eltype out Int64

julia> f(UInt32(1));
eltype in UInt32
eltype out Int64

julia> f(UInt64(1));
eltype in UInt64
eltype out UInt64

Although I can understand what happened here.

@KristofferC
Copy link
Sponsor Member

@sunoru This was in 0.5 to show the previous behavior. Promotion between scalars can be confusing sometimes yes. I think there is an issue dealing with it, with a suggestion to make it more intuitive, but I can't find it right now.

@sunoru
Copy link
Contributor

sunoru commented Mar 13, 2017

@KristofferC I see. I agree with you.

@JeffBezanson
Copy link
Sponsor Member

JeffBezanson commented Mar 13, 2017

If integers defaulted to Int32 everywhere there would be a large risk of things breaking when data size goes over 2GB, which is really common these days, and getting more common all the time. Attempting to fix that while still using Int32 for literals could cause nuissance type instabilities. We'd be spending a lot of time playing 2GB-barrier-whack-a-mole.

lower it to x = oftype(x, x ⨳ y) instead of just x = x ⨳ y. One would probably be hard pressed to find a case where changing the type of x in these kinds of expressions isn't a bug.

Of course, that could imply making an extra copy of an array, which would be unfortunate. If we really think these cases are bugs, maybe a type assertion instead?

@JeffBezanson
Copy link
Sponsor Member

This change does make it harder to write code that works the same on both 32 and 64-bit systems.
I've been playing around with this, and the results don't really mesh with my intuition. Some interesting examples--can you predict the output types on 32-bit and 64-bit systems?:

I would say that has more to do with .+= being in-place while .+ and += are out-of-place, than with the choice of promotion rules.

@una-dinosauria
Copy link

Ran into this issue on 0.6 and found another error when the .+= is in a return:

julia> function zero_plus_one()
       a =zeros(UInt32, 2)
       return a .+= 1
       end
zero_plus_one (generic function with 1 method)

julia> zero_plus_one()
2-element Array{UInt32,1}:
 0x00000001
 0x00000001

julia> function zero_plus_one()
       a =zeros(UInt32, 2)
       return 3, a .+= 1
       end
zero_plus_one (generic function with 1 method)

julia> zero_plus_one()
ERROR: MethodError: no method matching broadcast!(::##9#10, ::Tuple{Int64,Array{UInt32,1}}, ::Tuple{Int64,Array{UInt32,1}})
Closest candidates are:
  broadcast!(::Any, ::AbstractArray, ::Any, ::Any...) where N at broadcast.jl:204
  broadcast!(::Tf, ::Union{SparseMatrixCSC, SparseVector}) where Tf at sparse/higherorderfns.jl:98
  broadcast!(::Tf, ::Union{SparseMatrixCSC, SparseVector}, ::Union{SparseMatrixCSC, SparseVector}, ::Union{SparseMatrixCSC, SparseVector}...) where {Tf, N} at sparse/higherorderfns.jl:111
  ...
Stacktrace:
 [1] zero_plus_one() at ./REPL[10]:3

Hopefully this is not expected.

@yuyichao
Copy link
Contributor

The lowering is the same on 0.5. It is assigning to 3, a use (a .+=1) instead.

@sbromberger
Copy link
Contributor Author

what if you change

return 3, a .+= 1

to

return 3, (a .+= 1)

?

julia> function zpo()
       a = zeros(UInt32, 2)
       return 3, (a .+= 1)
       end
zpo (generic function with 1 method)

julia> zpo()
(3, UInt32[0x00000001, 0x00000001])

I think the issue is that the precedence for , is higher than the precendence for .+=.

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

No branches or pull requests