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

Avoid row promotion in heterogeneous arrays #24017

Merged
merged 1 commit into from
Nov 14, 2017

Conversation

pabloferz
Copy link
Contributor

Fixes #23994

This is the shameless obvious fix to the issue. I'll leave making hvcat more efficient (including optimizing based on inference when possible) for another occasion.

@pabloferz
Copy link
Contributor Author

pabloferz commented Oct 6, 2017

Of course the obvious fix doesn't work, so I went the second obvious fix.

Now the problem is that if I leave the methods definitions within the @testset it segfaults during testing. @JeffBezanson Is there a problem using the eltype of the Vararg arguments as in here?

EDIT: I was guessing it wouldn't be the case as I don't think some one is going to hvcat a large number of arguments by hand.

@pabloferz
Copy link
Contributor Author

@nanosoldier runbenchmarks(ALL, vs=":master")

@mbauman
Copy link
Member

mbauman commented Oct 6, 2017

I believe this will fail with

julia> [[1 2]; 3 4]
2×2 Array{Int64,2}:
 1  2
 3  4

I'm not as familiar with this code; can we just do:

hvcat(rows::Tuple{Vararg{Int}}, xs::Number...) = typed_hvcat(promote_typeof(xs...), rows, xs...)
hvcat(rows::Tuple{Vararg{Int}}, as...) = typed_hvcat(promote_eltypeof(as...), rows, as...)

@pabloferz
Copy link
Contributor Author

can we just do:

Yes, I believe something along those lines should work. Let me try again.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@pabloferz
Copy link
Contributor Author

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@pabloferz
Copy link
Contributor Author

Nanosoldier regressions seem noise, but just for kicks: @nanosoldier runbenchmarks("array" || "linalg", vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: stored type BenchmarkTools.ParametersPreV006 does not match currently loaded type

Logs and partial data can be found here
cc @ararslan

@kshyatt kshyatt added the arrays [a, r, r, a, y, s] label Oct 16, 2017
@kshyatt
Copy link
Contributor

kshyatt commented Oct 16, 2017

Looks like nanosoldier is down for a bit - do we want to merge anyway?

@ararslan
Copy link
Member

Someone could manually verify any slowdowns that Nanosoldier noted on its last successful run. Nanosoldier is indeed down for the count due to an outstanding issue with JLD.

@pabloferz
Copy link
Contributor Author

@nanosoldier runbenchmarks("array" || "linalg", vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@pabloferz
Copy link
Contributor Author

I believe the regressions are the same noisy ones observed all over the place. This should be good to go.

@pabloferz pabloferz merged commit 0b2bce6 into JuliaLang:master Nov 14, 2017
@pabloferz pabloferz deleted the pz/anyhvcat branch November 14, 2017 18:24
ararslan pushed a commit that referenced this pull request Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heterogeneous Array literals promote row by row
5 participants