-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
More efficient hvcat of scalars and arrays of numbers #39729
base: master
Are you sure you want to change the base?
Conversation
Oops, yep. completely broken. Should have guessed. |
This is what I achieved by consolidating with hvncat (once it's ready). Note it integrates #39725, which is currently broken for assigning strings into a range setindex. |
db3547c
to
de41ca2
Compare
b09fd6f
to
e5b9ba7
Compare
Updated this, now that the
|
With respect to the original motivating examples:
So better. Still some more improvements to make. Not entirely sure why the middle case is relatively poor. |
This comment was marked as outdated.
This comment was marked as outdated.
e41aa39
to
f8cb0ff
Compare
4d1c1d2
to
34872cd
Compare
Current status of this PR vs. Master: (Some timing differences from previous test likely due to my new computer) This PR:
Master:
So currently, Master is actually faster or equivalent for 4 of the 6 cases now, which is great. So will need to think if this PR is truly necessary anymore, and see what can improve that last case. |
Updating based on current master: This PR:
Master:
All cases on master are all faster than they were, even in the presence of additional allocations. I'll see about whether this PR can be wrapped up one way or the other this weekend I think. |
New simple approach - just dispatching hvcat to the hvncat_shape methods. Back to being similar or faster than master. But need to verify I didn't slow any other conditions down. EDIT: updated timings after better addressing dynamic dispatch of cat_similar
|
The remaining slowdown I'm seeing relates to the In 1.10, On master, the fallback abstractarrays.jl Possibly related to #49322 ? Looks like a regression in
But we can probably ignore that for the purposes of this PR, which is about hvcat. |
I wonder if this resolves the following issue: using LinearAlgebra
function measure()
data = rand(3,3)
mat = [
data ones(size(data, 1))
ones(size(data, 2))' 0
]
det(mat)
end
@code_warntype measure() # -> can't infer type of `mat` There has been some discussion at https://julialang.zulipchat.com/#narrow/stream/225542-helpdesk/topic/Matrix.20comprehension.20failed.20type.20inference. |
Seems like it does!
|
First attempt to address #39713
Original:
New:
Others unchanged, as expected.
Though if different types of numbers are mixed, it still takes the longer path. I tried expanding the definition but there's some weird stuff going on that increases allocations in the other situations I posted in that issue.Works for any Number element type.Fixes #39713