RFC: merging range indexes for faster subarray linear indexing #3778
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I doubt that any sane person will want to review the index gymnastics here, but this provides huge speed improvements for certain types of SubArray operations. It implements a commented TODO (which I deleted a couple of days ago, so I figured I better do it!): using range indexes, when possible, for trailing linear indexing dimensions. It's an optimization that is only applicable under certain conditions, but those conditions arise fairly commonly in practice (e.g., array slices).
Previously:
(My commit from earlier this morning made this already ~2.5x faster than it was yesterday.)
With this code:
So about a 100-fold speed improvement, not bad, and indeed it's as fast as
A[:,:,3]
.The algorithmic details are not fun, but comments on the overall architecture and naming might be useful. For instance, there's a fair amount of overlap between these functions; would it be better to merge them and return
1:0
if the indexes are not mergeable? (One downside is that there would be some additional computational burden on cases where merging fails.) Note also that the code returns either aRange1
orRange
, and perhaps that's a bad idea? Finally, feel free to suggest any additional tests that I might have missed.