Skip to content

Conversation

martinholters
Copy link
Collaborator

Mimic the behavior for Arrays and only use return_type in the empty case. I wonder, however, whether we shouldn't use Union{} in the empty case. We don't have to worry about the type-instability as the size is part of the type, too, and cannot e.g. push! to an SVector. So there seems to be little harm. Opinions?

Ref. JuliaLang/julia#28981, closes #493. But note that nested broadcasts will still suffer from incomplete inference due to the recursion limiting heuristics.

Mimic the behavior for `Array`s and only use `return_type` in the empty 
case.
@martinholters martinholters changed the title Determine broadcast element type base on actual values Determine broadcast element type based on actual values Sep 14, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 95.0% when pulling 92f707b on martinholters:mh/broadcast_eltype into 81dd6ca on JuliaArrays:master.

@test_broken eltype(a - 2) == Number
@test_broken eltype(a * 2) == Number
@test_broken eltype(a / 2) == Number
@test eltype(a + 2) == Number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these methods be deprecated? They are for general arrays.

julia> [1, 2, 3] + 1
ERROR: MethodError: no method matching +(::Array{Int64,1}, ::Int64)
Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...) at operators.jl:502
  +(::Complex{Bool}, ::Real) at complex.jl:292
  +(::Missing, ::Number) at missing.jl:93
  ...
Stacktrace:
 [1] top-level scope at none:0

julia> @SVector([1, 2, 3]) + 1
3-element SArray{Tuple{3},Int64,1,3}:
 2
 3
 4

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but orthogonal to this issue, except that I could use the explicit broadcast in these tests as I'm touching them anyways. (And it's in the broadcast tests.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, just wanted to point it out since I noticed it 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the tests locally; will push once I have feedback what to do about the empty case.

@andyferris
Copy link
Member

andyferris commented Sep 18, 2018

I wonder, however, whether we shouldn't use Union{} in the empty case.

It's a very interesting question. Here's a splattering of random thoughts.

  • I hate that we have to treat empty containers seperately at all. My belief is that robust systems should be able to gracefully and naturally handle the empty container case. (At the very least, we have to do something reasonable for our StaticArrays users, but this is a broader comment about Julia).
  • Parts of the Julia language and implementation (what I call the "covariant" parts) would certainly favor Union{} as the output element type. In a fully covariant system it is in-fact rather elegant, and works well with promote_type and so-on.
  • On the other hand, invariant type parameters means there's code all over the place which expects certain type parameters. If empty arrays were to come up, they might not match function signatures and so-on. As a small example, if we were to take the "covariant" approach to its extremes, I wonder how much code like f(::AbstractArray{Float64}) should actually be f(::AbstractArray{<:Float64}) (which is for all intents and purposes f(::Union{AbstractArray{Float64}, AbstractArrray{Union{}}), and the latter case would typically occur only for empty arrays).
  • Using Union{} in the empty case for static arrays shouldn't be a problem in practice. But as an example of the tension between covariance and invariance in Julia, if dynamically sized arrays did this then operations like filter couldn't possibly be type-stable.
  • I think this question can be resolved seperately to the issue at hand. :)

There is however a very important "covariance vs invariance" discussion involving the work that is in this PR, relating to the whole DataValue{T} vs Union{Missing, T} debate. Switching to covariance means that broadcast kernels (i.e. f in f.(a)) that return Union{Missing, T} can result in arrays of final element type Missing, T2, or Union{Missing, T2} (where f(::T) --> T2). I would be tempted to describe this as the type uncertainty spreading in a non-deterministic fassion, and my staic compilation background makes me prefer that we just get Union{Missing, T2}. What actually matters here is (a) what's the performance difference between these two choices and (b) is it likely that this type uncertainty will spread further downstream, or will static analysis of programs with complex data flow including missing values tend to remain capped at Unions of 3 different types? I'm interested in everyone's thoughts on this.

@martinholters
Copy link
Collaborator Author

Ok, let's leave the discussion of the empty case to another time.

So yeah, with this PR idenitiy.(x::SVector{N,Union{Missing, T}}) can give SVector{N,Union{Missing, T}}, SVector{N,Missing}, or SVector{N,T}, and is inferred as SVector{N}. Yes, I agree, that does not sound too attractive.
But without this PR, it gives SVector{N,R} where R is hopefully (!) some supertype of Union{Missing, T}, depending on how inference is feeling today, and is inferred as such. That does not sound too much better to me. And the "hopefully" there (see JuliaLang/julia#28981) makes it far worse, actually. Of course, this usually works fine, even finding the "correct" (tightest) element type. But there is no guarantee, and I don't think it's possible to decide whether we are in good or a bad case.

andyferris pushed a commit that referenced this pull request Sep 20, 2018
Solve various inference issues. Replaces #495 and #330.
Closes #493.

Note: the eltype of empty arrays may become `Union{}` in
`map`, `broadcast`, and `mapreduce(...; dims = Val{N})`.
@andyferris andyferris mentioned this pull request Sep 20, 2018
@andyferris
Copy link
Member

See new PR #503. It's also worth noting #330.

andyferris pushed a commit that referenced this pull request Sep 20, 2018
Solve various inference issues. Replaces #495 and #330.
Closes #493.

Note: the eltype of empty arrays may become `Union{}` in
`map`, `broadcast`, and `mapreduce(...; dims = Val{N})`.
@andyferris
Copy link
Member

Done by #503.

@andyferris andyferris closed this Sep 20, 2018
@martinholters martinholters deleted the mh/broadcast_eltype branch September 20, 2018 14:04
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

Successfully merging this pull request may close these issues.

A segfault with member functions
4 participants