Remove final explicit uses of Inference.return_type #330

c42f · 2017-11-01T10:10:53Z

With this approach based on the eltype of tuple of computed elements, we get the correct dynamic result independently of whether inference can infer the return type.

This fixes #198, and partially addresses #329.

With the addition of a workaround hack to override a very specific version of broadcast in a very specific way it also fixes #329.

codecov-io · 2017-11-01T10:29:31Z

Codecov Report

Merging #330 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
- Coverage   93.14%   93.13%   -0.01%     
==========================================
  Files          37       37              
  Lines        2654     2651       -3     
==========================================
- Hits         2472     2469       -3     
  Misses        182      182

Impacted Files	Coverage Δ
src/broadcast.jl	`100% <100%> (ø)`	⬆️
src/mapreduce.jl	`99.16% <100%> (-0.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 400fd9c...0a78009. Read the comment docs.

coveralls · 2017-11-01T10:29:41Z

Coverage decreased (-0.005%) to 93.135% when pulling 0d292c9 on cjf/broadcast-inference-fix into 715fefe on master.

andyferris

Nice. Out of curiousity, was does it do for things that are like Union{Null, T} on v0.6 and v0.7?

andyferris · 2017-11-01T11:20:15Z

src/broadcast.jl

    return quote
        @_inline_meta
-        @inbounds return similar_type($first_staticarray, $newtype_expr, Size($newsize))(tuple($(exprs...)))
+        elements = tuple($(exprs...))
+        @inbounds return similar_type($first_staticarray, eltype(elements), Size($newsize))(elements)


eltype(::Tuple) is (or at least, was) a bit pesimised. In util.jl there is a function called promote_tuple_eltype that I originally wrote for this kind of stuff. I'm not sure what the current status on which one is better (and it's possible that reducing via typejoin is a better choice here than promote_type anyway??)

Interesting. I didn't give type reduction a lot of thought TBH, because I'm expecting all the elements to have the same inferred type (How could they not? They're all copies of the same expression unrolled many times.)

In the known bad cases which required the broadcast workaround, the issue seems to be in inferring the type of the element tuple, rather than in the typejoin step.

c42f · 2017-11-01T20:05:12Z

What does it do for Union{T,Null}

An excellent question. On this branch we have (for both 0.7 and 0.6):

julia> broadcast(+, SVector(1.0,null), SVector(2.0,3.0))
2-element SVector{2,Any}:
 3.0
  null

Where as, with current StaticArrays master, we have

julia> broadcast(+, SVector(1.0,null), SVector(2.0,3.0))
2-element SVector{2,Union{Float64, Nulls.Null}}:
 3.0
  null

which is arguably better (though probably neither is useful on 0.6).

I wonder whether it's a problem with the way I've done things on this branch, or a flaw in the "use Union{Null,T} for missingness" idea? Basically, by using the actual runtime type of the elements via eltype, we can compute a more precise container type for the SVector at runtime, but at the cost of a much less precise type at compile time (thinking that Union{Null,T} wil be efficient in 0.7). This doesn't sound like a win :-(

I guess, if nothing else, what I've done here is to bring these methods in line with the rest of StaticArrays, where we generally infer the static array type from the element tuple.

I'm now not quite sure whether to merge this!

c42f · 2017-11-01T21:22:22Z

If I replace eltype with promote_tuple_eltype, we have

julia> broadcast(+, SVector(1.0,null), SVector(2.0,3.0))
2-element SVector{2,Union{Float64, Nulls.Null}}:
 3.0
  null

which is an improvement, i think. It's still not type inferrable though.

What do you think about Union Andy? Is it a problem for another time? It seems like we'd have to reassess many things in the package if we wanted to allow possible Nulls to propagate through in a nice way.

c42f · 2017-11-01T21:44:05Z

Ok, I've pushed a change to use promote_tuple_type instead.

The broadcast workaround doesn't seem necessary on 0.7, so presumably the associated inference problem has been sorted out in Base.

andyferris · 2017-11-02T00:42:38Z

Yeah, Null and Union is a pain. I'm not sure what the correct answer is here (sorry).

c42f · 2017-11-02T06:08:38Z

Hmm, looks like I've broken the test case which asserts that StaticArrays shares the Base behaviour when you have an array of Number. Ie,

julia> Number[2, 2.0, 4//2, 2+0im] .+ 2
4-element Array{Number,1}:
  4
  4.0
 4//1
 4+0im

With promote_tuple_eltype, the resulting array in the StaticArrays case is a Complex{Float64} instead.

Honestly, I'm not sure this really matters, and perhaps I could just remove that test case: having an SArray of abstract types like Number seems a bit useless.

coveralls · 2017-11-02T20:47:17Z

Coverage decreased (-0.005%) to 93.135% when pulling 98a7e1d on cjf/broadcast-inference-fix into 715fefe on master.

With this approach based on the eltype of tuple of computed elements, we get the correct dynamic result independently of whether inference can infer the return type. This fixes some of the residual issues from #198.

This allows `Union`s as the output type for mixtures of `Null` and numeric types, which can be more efficient than the `Any` you'd get from typejoin, at least in 0.7. It also makes `SVectors` of abstract types like `Number` become concrete after numerical operations, which is inconsistent with base, but probably not a problem for realisitc uses of this package.

andyferris · 2017-11-03T00:58:43Z

I have this feeling that map and broadcast should be widening the eltype until all elements match, not promoting them at the end (and therefore potentially changing them). Does that make sense? Maybe we need a typejoin_tuple_eltype-kind-of-thing?

coveralls · 2017-11-03T01:21:59Z

Coverage decreased (-0.008%) to 93.135% when pulling 0a78009 on cjf/broadcast-inference-fix into 400fd9c on master.

c42f · 2017-11-03T01:47:00Z

Yes, I was just about to suggest that we might need our own purpose built typejoin-like thing :-) We can then tweak it to the needs of the users of this package.

Regarding promotion vs typejoinn - we do already use promotion in similar circumstances:

julia> SVector(1, 1.0)
2-element SVector{2,Float64}:
 1.0
 1.0

It's arguably a similar kind of thing, and certainly much more predictable and useful behavior from a performance standpoint. (We follow Base here, which has the same array construction conventions.)

I do think that it would be nice if arithmetic on SVector{N,Union{Null,T}} preserved the union type, which is how things currently work when we rely directly on calling return_type. Thinking julia 0.7 here, the eltype based version would be a bit of a performance disaster in this case. Consider SVector(1, null) .+ 1. When using the typejoin version this expands to:

a = SVector(1,null)
# inferred: a::SVector{2,Union{Int,Null}}
# runtime: a::SVector{2,Union{Int,Null}}
elements = (1 + a[1], 1 + a[2])
# inferred: elements::Tuple{Union{Int, Null},Union{Int, Null}}
# runtime: elements::Tuple{Int,Null}
newtype = eltype(elements)
# inferred: newtype::Type
# runtime: newtype::Union{Int,Null}
newvec = similar_type(a, newtype)(elements)
# inferred: newvec::SVector{2,_} where _
# runtime: newvec::SVector{2,Union{Int,Null}}

[edit - fixed mistakes]

Unfortunately, inferring SVector{2,_} where _ for the output seems pretty impractical as it will completely destroy the performance. I wonder whether there's a way around this which makes sense (other than what we already do)?

andyferris · 2018-09-19T23:13:16Z

I think the time for this has finally come.

See #495, #493 and JuliaLang/julia#28981.

I think I'll make an updated version of this PR this soon. Upon reflection on what Base does, it appears we should use eltype not promote_tuple_eltype for the eltype calculation. Basically, unlike literal construction of arrays, operations like map and broadcast don't tend to promote all the elements to a common type. E.g.

julia> map(-, Union{Float64, Int}[1, 1.0])
2-element Array{Real,1}:
 -1  
 -1.0

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 · 2018-09-20T09:57:26Z

Implemented by #503

c42f mentioned this pull request Nov 1, 2017

Type inference failure on round broadcast #329

Closed

andyferris approved these changes Nov 1, 2017

View reviewed changes

c42f force-pushed the cjf/broadcast-inference-fix branch from 16ee6bc to 98a7e1d Compare November 2, 2017 19:44

c42f mentioned this pull request Nov 2, 2017

Workaround inference problem in round.(Int,v) #332

Merged

c42f added 2 commits November 3, 2017 10:39

Remove final uses of Inference.return_type

4231a73

With this approach based on the eltype of tuple of computed elements, we get the correct dynamic result independently of whether inference can infer the return type. This fixes some of the residual issues from #198.

c42f force-pushed the cjf/broadcast-inference-fix branch from 98a7e1d to 0a78009 Compare November 3, 2017 00:40

andyferris pushed a commit that referenced this pull request Sep 20, 2018

Fix all the things

20d7a96

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})`.

This was referenced Sep 20, 2018

Fix all the things #503

Merged

Determine broadcast element type based on actual values #495

Closed

andyferris pushed a commit that referenced this pull request Sep 20, 2018

Fix all the things

03165ba

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 closed this Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove final explicit uses of Inference.return_type #330

Remove final explicit uses of Inference.return_type #330

c42f commented Nov 1, 2017

codecov-io commented Nov 1, 2017 •

edited

Loading

coveralls commented Nov 1, 2017

andyferris left a comment

andyferris Nov 1, 2017

c42f Nov 1, 2017

c42f commented Nov 1, 2017

c42f commented Nov 1, 2017

c42f commented Nov 1, 2017

andyferris commented Nov 2, 2017

c42f commented Nov 2, 2017 •

edited

Loading

coveralls commented Nov 2, 2017

andyferris commented Nov 3, 2017

coveralls commented Nov 3, 2017

c42f commented Nov 3, 2017 •

edited

Loading

andyferris commented Sep 19, 2018

andyferris commented Sep 20, 2018

Remove final explicit uses of Inference.return_type #330

Remove final explicit uses of Inference.return_type #330

Conversation

c42f commented Nov 1, 2017

codecov-io commented Nov 1, 2017 • edited Loading

Codecov Report

coveralls commented Nov 1, 2017

andyferris left a comment

Choose a reason for hiding this comment

andyferris Nov 1, 2017

Choose a reason for hiding this comment

c42f Nov 1, 2017

Choose a reason for hiding this comment

c42f commented Nov 1, 2017

c42f commented Nov 1, 2017

c42f commented Nov 1, 2017

andyferris commented Nov 2, 2017

c42f commented Nov 2, 2017 • edited Loading

coveralls commented Nov 2, 2017

andyferris commented Nov 3, 2017

coveralls commented Nov 3, 2017

c42f commented Nov 3, 2017 • edited Loading

andyferris commented Sep 19, 2018

andyferris commented Sep 20, 2018

codecov-io commented Nov 1, 2017 •

edited

Loading

c42f commented Nov 2, 2017 •

edited

Loading

c42f commented Nov 3, 2017 •

edited

Loading