Make promote_op rely on Core.Inference.return_type #17389

pabloferz · 2016-07-12T14:26:48Z

This should fix #17314 and the problem found in #17394

EDIT: This also removes all specialized promote_op methods, since the new ones should handle all previous cases.

pabloferz · 2016-07-12T14:31:44Z

@martinholters After this, you should be able to use broadcast in #17313 as you originally intended.

martinholters · 2016-07-12T14:38:09Z

base/arraymath.jl

            for (iF, iB) in zip(eachindex(F), eachindex(B))
                @inbounds F[iF] = ($f)(A, B[iB])
            end
            return F
        end
        function ($f){T}(A::AbstractArray{T}, B::Number)
-            F = similar(A, promote_array_type($f,typeof(A),typeof(B)))
+            F = similar(A, promote_op($f, T, typeof(B)))


promote_array_type is responsible for e.g. typeof(Float32[1,2,3]*1.0) == Vector{Float32}, even though Base.promote_op(*, Float32, typeof(1.0)) == Float64 and typeof(Float32(1)*1.0) == Float64. If you look at above machinery closer, you'll note that the result of promote_op is only used in a few cases, and that's for a reason.

martinholters · 2016-07-12T14:44:20Z

After this, you should be able to use broadcast in #17313 as you originally intended.

How is that? The reason for not not using broadcast all the time in #17313 was that it incurs some constant overhead even if promote_op returns a perfectly valid result, ruining performance for small arrays. Actually, if this PR "fixes" promote_op to more often do the right thing (and makes it into 0.5), #17313 would be obsolete.

pabloferz · 2016-07-12T14:52:59Z

The reason for not not using broadcast all the time in #17313 was that it incurs some constant overhead even if promote_op returns a perfectly valid result, ruining performance for small arrays.

Sorry, I overlooked that in #17313.

Actually, if this PR "fixes" promote_op to more often do the right thing (and makes it into 0.5), #17313 would be obsolete.

Well, this should make promote_op do the right thing more often. Though I didn't clean up the methods in broadcast.jl here.

tkelman · 2016-07-13T00:01:21Z

There's been resistance in the past to making program behavior (as opposed to just performance) depend on the results of inference.

TotalVerb · 2016-07-13T02:47:58Z

I understand that sentiment, but something needs to be done about promote_op. What about the compromise of using it only in the empty case?

JeffBezanson · 2016-07-13T03:08:10Z

The ideal situation is to use the inference result when it is concrete, and otherwise use the types of the actual values. Then there is only a problem in the empty case. Using the inferred type there is deemed acceptable for now.

pabloferz · 2016-07-13T16:00:27Z

@JeffBezanson Does the current proposal address your concerns (at least provisionally and until we have a better mechanism)?

Tests are now passing on AV (at the moment Travis has a very long queue) and I don't think this should affect performance in general. Of course, that should be checked.

tkelman · 2016-07-13T16:43:39Z

test/broadcast.jl

@@ -164,8 +164,6 @@ 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{typeof(.+), Array{Float64, 3}, Array{Int, 3}})
-@test length(rt) == 1 && rt[1] == Array{Float64, 3}


why is this deleted?

With these changes broadcast wouldn't be inferable (as map) but it will return the correct type nevertheless. I guess I should change it to at least verify that. <- Done

pabloferz · 2016-07-14T08:31:27Z

The Travis failure seems to be due to #16537 (see #17409)

tkelman · 2016-07-16T14:05:42Z

otherwise use the types of the actual values

This isn't doing the latter, it's returning Any when inference doesn't return a leaf type?

pabloferz · 2016-07-16T14:24:35Z

@tkelman You're right, but I believe the cases where the type returned by return_type is not concrete won't be the ones where we care about performance, so we can use the fall back broadcast method.

Actually, this is more in analogy with the _default_eltype used by map (through, the collect methods). So in a way broadcast and map would behave alike with this changes (this was what @stevengj was advocating IIUC).

If the first statement above is not accurate I can change the PR to accommodate Jeff's initial comment.

yuyichao · 2016-07-16T14:27:47Z

It'll be funny if we introduce another type inference dependent behavior for non-empty cases right after we worked really hard to fix one....

pabloferz · 2016-08-02T13:51:52Z

I don't think so. What was the comment about?

tkelman · 2016-08-02T13:57:25Z

I could have sworn that a few days ago I said I didn't think we should merge this until after we branch, though that was before seeing the issues raised in the last few days, and I need to look at the pkgeval results.

pabloferz · 2016-08-02T13:59:35Z

This #16986 (comment)? I guess you misplaced it.

tkelman · 2016-08-02T13:59:53Z

I was thinking of #16986, nevermind, sorry.

tkelman · 2016-08-02T23:22:59Z

Sorry for the delay, took me a while to get in front of a proper keyboard.

This turns out to be hugely breaking in packages: https://gist.github.com/8f19fefaf5021dbc076cc47b3ff8abd1

Could use a hand digging through those json logs (jq and/or JSON.jl are useful for this) to see if they're all the same failure though, maybe a re-run now that DataArrays has been tagged might fix them all?

tkelman · 2016-08-03T07:52:15Z

With DataArrays tagged it's a much smaller number of failures now. https://gist.github.com/5049d028f493ef2a9163a269f9a634fc
Just BandedMatrices (which you have a fix for), GaussianMixtures, and NearestNeighbors. Could you take a look at the other 2?

(Reactive has flaky tests, and apparently this PR actually fixes SampledSignals http://pkg.julialang.org/detail/SampledSignals.html)

tkelman · 2016-08-03T08:48:36Z

signal (15): Terminated\nwhile loading no file, in expression starting on line 0\n\nsignal (15): Terminated\nwhile loading /home/vagrant/.julia/v0.5/NearestNeighbors/test/test_monkey.jl, in expression starting on line 6\nuv__epoll_wait
I think that means NearestNeighbors' tests got slower with this change and they timed out.

KristofferC · 2016-08-03T09:09:03Z

Deprecation warnings? I fixed deprecations now so maybe try again (with newest commit).

tkelman · 2016-08-03T09:18:59Z

PkgEval runs on tagged package versions.

KristofferC · 2016-08-03T09:20:56Z

Ok, I tag then.

pabloferz · 2016-08-03T10:27:07Z

The NearestNeighbors one is fixed (locally) for me by fixing the deprecations as @KristofferC pointed out.

On the other hand I'm not that sure how this is affecting GaussianMixtures, it passed the first time, but it errored the second (on a function that has nothing to do with DataArrays). Also, it passes the tests on my machine. I suspect the memory used fluctuates around the edges of the memory of the testing machine, but I'm not sure.

tkelman · 2016-08-03T10:49:19Z

Ah right it's an OutOfMemoryError() so possibly unrelated. Do you see a noticeable difference in peak memory use on this branch vs master?

pabloferz · 2016-08-03T10:53:36Z

Do you see a noticeable difference in peak memory use on this branch vs master?

I don't. But you already merged it, so I guess there is no problem.

tkelman · 2016-08-03T11:14:07Z

There might be a problem, but I'll take one fixed package (plus 2 about-to-be-fixed packages) in exchange for one broken one. cc @davidavdav

timholy · 2016-08-03T12:06:50Z

Reactive has flaky tests

That might have been fixed by JuliaGizmos/Reactive.jl@d7a2bd3, which was tagged just a couple of days ago.

tkelman · 2016-08-03T12:13:15Z

Not really http://pkg.julialang.org/detail/Reactive.html

JeffBezanson · 2016-08-03T22:09:49Z

base/promotion.jl

+if isdefined(Core, :Inference)
+    function _promote_op(op, T::Type)
+        G = Tuple{Generator{Tuple{T},typeof(op)}}
+        R = Core.Inference.return_type(first, G)


Does return_type(op, Tuple{T}) not work? This seems roundabout.

That was the first thing I tried, the problem is that Test.@inferred does not work over _promote_op that way, but it works with this.

This reverts commit 3ace05a. ref #17389

[release-0.5] revert #17389 for 0.5.0-rc1

This reverts commit dbdaf72. ref #17389 and #17929

This reverts commit e6f5d89. ref #17798 and #17389 and #17929

…eps/build.jl.

martinholters reviewed Jul 12, 2016
View reviewed changes

pabloferz force-pushed the pz/pobc branch 5 times, most recently from f1b0edf to 2464ebc Compare July 12, 2016 23:07

pabloferz changed the title ~~Make promote_op rely on Core.Inference.return_type~~ Make broadcast rely on Core.Inference.return_type Jul 12, 2016

pabloferz force-pushed the pz/pobc branch 3 times, most recently from 4a48215 to 3803ee4 Compare July 13, 2016 14:44

pabloferz mentioned this pull request Jul 13, 2016

Remove eltype_plus #17398

Merged

tkelman reviewed Jul 13, 2016
View reviewed changes

pabloferz force-pushed the pz/pobc branch from 01fc529 to 38418f5 Compare July 13, 2016 17:20

pabloferz force-pushed the pz/pobc branch 2 times, most recently from 950492e to 85d5f22 Compare July 14, 2016 09:31

pabloferz mentioned this pull request Jul 14, 2016

promote_op/broadcast should not call the function for one(T) #17314

Closed

pabloferz force-pushed the pz/pobc branch 3 times, most recently from b70468b to 96e364a Compare July 16, 2016 13:57

stevengj added the broadcast Applying a function over a collection label Aug 2, 2016

tkelman mentioned this pull request Aug 3, 2016

RFH: 0.5-style comprehensions broke the way you're using testsets KristofferC/NearestNeighbors.jl#25

Closed

tkelman merged commit 121aa36 into JuliaLang:master Aug 3, 2016

pabloferz deleted the pz/pobc branch August 3, 2016 10:59

stevengj mentioned this pull request Aug 3, 2016

huge performance regression in vector math #17794

Closed

JeffBezanson reviewed Aug 3, 2016
View reviewed changes

tkelman mentioned this pull request Aug 4, 2016

Checklist of steps towards 0.5 release [candidates] #17418

Closed

16 tasks

tkelman added a commit that referenced this pull request Aug 4, 2016

Revert "Make promote_op rely on Core.Inference.return_type"

dbdaf72

This reverts commit 3ace05a. ref #17389

tkelman added a commit that referenced this pull request Aug 4, 2016

Merge pull request #17804 from JuliaLang/tk/revert-promoteop

b3e4722

[release-0.5] revert #17389 for 0.5.0-rc1

stevengj mentioned this pull request Aug 4, 2016

Unusal handling of Integer typed empty arrays #17811

Closed

tkelman mentioned this pull request Aug 10, 2016

Improve promote_op #17929

Merged

tkelman added a commit that referenced this pull request Aug 20, 2016

Revert "Revert "Make promote_op rely on Core.Inference.return_type""

4160a8a

This reverts commit dbdaf72. ref #17389 and #17929

tkelman added a commit that referenced this pull request Aug 20, 2016

Revert "Revert "Restore type parameter for elementwise ops""

b9d7579

This reverts commit e6f5d89. ref #17798 and #17389 and #17929

ajkeller34 referenced this pull request in PainterQubits/Unitful.jl Feb 21, 2017

Some patches so that Unitful still works on julia 0.5; also, update d…

f03dbcd

…eps/build.jl.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make promote_op rely on Core.Inference.return_type #17389

Make promote_op rely on Core.Inference.return_type #17389

pabloferz commented Jul 12, 2016 •

edited

Loading

pabloferz commented Jul 12, 2016

martinholters Jul 12, 2016

martinholters commented Jul 12, 2016

pabloferz commented Jul 12, 2016 •

edited

Loading

tkelman commented Jul 13, 2016

TotalVerb commented Jul 13, 2016

JeffBezanson commented Jul 13, 2016

pabloferz commented Jul 13, 2016 •

edited

Loading

tkelman Jul 13, 2016

pabloferz Jul 13, 2016 •

edited

Loading

pabloferz commented Jul 14, 2016

tkelman commented Jul 16, 2016

pabloferz commented Jul 16, 2016

yuyichao commented Jul 16, 2016 •

edited

Loading

pabloferz commented Aug 2, 2016

tkelman commented Aug 2, 2016

pabloferz commented Aug 2, 2016

tkelman commented Aug 2, 2016

tkelman commented Aug 2, 2016

tkelman commented Aug 3, 2016

tkelman commented Aug 3, 2016 •

edited

Loading

KristofferC commented Aug 3, 2016 •

edited

Loading

tkelman commented Aug 3, 2016

KristofferC commented Aug 3, 2016

pabloferz commented Aug 3, 2016 •

edited

Loading

tkelman commented Aug 3, 2016

pabloferz commented Aug 3, 2016

tkelman commented Aug 3, 2016

timholy commented Aug 3, 2016

tkelman commented Aug 3, 2016

JeffBezanson Aug 3, 2016

pabloferz Aug 3, 2016 •

edited

Loading

Make promote_op rely on Core.Inference.return_type #17389

Make promote_op rely on Core.Inference.return_type #17389

Conversation

pabloferz commented Jul 12, 2016 • edited Loading

pabloferz commented Jul 12, 2016

martinholters Jul 12, 2016

Choose a reason for hiding this comment

martinholters commented Jul 12, 2016

pabloferz commented Jul 12, 2016 • edited Loading

tkelman commented Jul 13, 2016

TotalVerb commented Jul 13, 2016

JeffBezanson commented Jul 13, 2016

pabloferz commented Jul 13, 2016 • edited Loading

tkelman Jul 13, 2016

Choose a reason for hiding this comment

pabloferz Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

pabloferz commented Jul 14, 2016

tkelman commented Jul 16, 2016

pabloferz commented Jul 16, 2016

yuyichao commented Jul 16, 2016 • edited Loading

pabloferz commented Aug 2, 2016

tkelman commented Aug 2, 2016

pabloferz commented Aug 2, 2016

tkelman commented Aug 2, 2016

tkelman commented Aug 2, 2016

tkelman commented Aug 3, 2016

tkelman commented Aug 3, 2016 • edited Loading

KristofferC commented Aug 3, 2016 • edited Loading

tkelman commented Aug 3, 2016

KristofferC commented Aug 3, 2016

pabloferz commented Aug 3, 2016 • edited Loading

tkelman commented Aug 3, 2016

pabloferz commented Aug 3, 2016

tkelman commented Aug 3, 2016

timholy commented Aug 3, 2016

tkelman commented Aug 3, 2016

JeffBezanson Aug 3, 2016

Choose a reason for hiding this comment

pabloferz Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

pabloferz commented Jul 12, 2016 •

edited

Loading

pabloferz commented Jul 12, 2016 •

edited

Loading

pabloferz commented Jul 13, 2016 •

edited

Loading

pabloferz Jul 13, 2016 •

edited

Loading

yuyichao commented Jul 16, 2016 •

edited

Loading

tkelman commented Aug 3, 2016 •

edited

Loading

KristofferC commented Aug 3, 2016 •

edited

Loading

pabloferz commented Aug 3, 2016 •

edited

Loading

pabloferz Aug 3, 2016 •

edited

Loading