JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.68k stars 5.48k forks source link

broadcasting has potentially type-unstable behavior on Adjoints #29569

Closed isaac-rstor closed 6 years ago

isaac-rstor commented 6 years ago
julia> a = [1, 2, 3]
3-element Array{Int64,1}:
 1
 2
 3

julia> a'
1×3 LinearAlgebra.Adjoint{Int64,Array{Int64,1}}:
 1  2  3

julia> b = [4, 5, 6]
3-element Array{Int64,1}:
 4
 5
 6

julia> b'
1×3 LinearAlgebra.Adjoint{Int64,Array{Int64,1}}:
 4  5  6

julia> a' .+ b'
1×3 Array{Int64,2}:
 5  7  9

julia> a' + b'
1×3 LinearAlgebra.Adjoint{Int64,Array{Int64,1}}:
 5  7  9

Not an issue for .+ (because can usually use + fearlessly) but potentially an issue for .*

jrevels commented 6 years ago

Sorry, I think I'm missing something - where is the type instability here?

isaac-rstor commented 6 years ago

sorry, the instability was in my original code, but not in the minimal error condition, if you are doing an update of a value, like:

adjoint_vector_x .+= adjoint_vector_y

nalimilan commented 6 years ago

I still don't get it. Can you post code showing the problem?

yuyichao commented 6 years ago

Dependency of output type on input type is not type instability

isaac-rstor commented 6 years ago
julia> a = [1,2,3]'
1×3 LinearAlgebra.Adjoint{Int64,Array{Int64,1}}:
 1  2  3

julia> b = [4,5,6]'
1×3 LinearAlgebra.Adjoint{Int64,Array{Int64,1}}:
 4  5  6

julia> a = a .+ b
1×3 Array{Int64,2}:
 12  20  30
isaac-rstor commented 6 years ago

ok surprisingly

a .+= b is fine (and I understand why).

@yuyichao you are correct, and that is why I said "potentially", because it becomes type instability when people do things that don't look like they should be unstable in places where they expect it to be stable.

yuyichao commented 6 years ago

This is certainly not a bug. There's never going to be a rule that function has to return the same type as it's argument. a = a .+ b is also not a potential type instability (not more than the potential type instability in a = b anyway.)

isaac-rstor commented 6 years ago

Uhm can we have more discussion on this? nobody is saying that there should be any generalizations about functions. I'm just saying that broadcast operators should probably respect the type of the input data, at least for builtin datatypes. If you want to create your own datatypes, fine, do what you want.

martinholters commented 6 years ago

Right. Although there is nothing type-unstable here, having broadcast over Adjoints return an Adjoint sounds like a reasonable idea. Unfortunately, changing a return type will have to wait for 2.0.

isaac-rstor commented 6 years ago

Thanks!

yuyichao commented 6 years ago

Well, that's impossible in general, since functions in general (including +) isn't closed and many types also have invariance that cannot be maintained across many operations (AbstractRange for example).

Proposing changing the output type on a case by case basis is fine. However, there's nothing special about broadcast here and there's no potential type instability.

isaac-rstor commented 6 years ago

I don't think you should dismiss that there's "no potential type instability" because I had type instability in an jupyter notebook when I was vectorizing some code from Andrew Ng's coursera course. Maybe I'm dumb, but I have been using julia for upwards of 4+ years, so at least I knew to look for it. Other people might not be so knowledgeable about the underlying mechanics and wonder why their vector has turned into a matrix.

Your being dismissive is incredibly unhelpful @yuyichao.

yuyichao commented 6 years ago

"no potential type instability" because I had type instability in an jupyter notebook when I was vectorizing some code from Andrew Ng's coursera course

Well, I said,

not more than the potential type instability in a = b anyway.

Sure, it is a trivial fact that there are infinite number of code transformations that can give you type instability and just as many ways to write type unstable code straight away. I'm saying that there's nothing more about introducing type instability above that trivial level in your code. There's nothing different from the type instability you are seeing from the classic s += v::Float64 one, in both case the assignment is responsible for it. In another word, the potential type instability is from the =, not the .+.

Other people might not be so knowledgeable about the underlying mechanics and wonder why their vector has turned into a matrix.

Well, I don't really see what's needed to know about underlying mechanics here. Type stability on variable happens on assignment by definition. That may not be obvious and learning that is the purpose of the discourse site.

dismissive

I am correcting your incorrect use of concept that has obviously caused confusion already.

And again, the close is because there's no potential type instability here. Reopen as proposal for changing this particular case is fine though I don't think there can be a general rule so documenting the behavior seems suffice as well.