MAINT: np.pad: Add helper functions for producing slices along axes#11012
MAINT: np.pad: Add helper functions for producing slices along axes#11012mhvk merged 2 commits intonumpy:masterfrom
Conversation
|
@eric-wieser What is the current status of this? |
|
Needs a base-switch from master and back again now that the other PR is merged, which will hide the first merged commit from the diff |
numpy/lib/arraypad.py
Outdated
There was a problem hiding this comment.
The alternative I was considering was _axis_slice(arr.shape, lambda n: slice(n-1, n), axis=axis), which covers a few more uses, but I wasn't sure if that was clearer.
There was a problem hiding this comment.
Not clearer as written, but I think it would be clearer if one were to just pass in the required slice itself. This is possible, since, as I noted above, it is not necessary to know the actual dimension; here, one would just pass in slice(-1, None).
numpy/lib/arraypad.py
Outdated
There was a problem hiding this comment.
One doesn't actually need the shape for either _begin_slice (obvious, it is not used), or _end_slice: just do
def _end_slice(ndim, n, axis):
return tuple(slice(None) if i != axis else slice(-n, None) for i in range(ndim))
There was a problem hiding this comment.
-n fails when n == 0. I'd like to remove some of the zero special-casing, if possible
There was a problem hiding this comment.
Actually, thinking more, we do not need ndim either, since you only need a tuple up to axis - all the later dimensions are automatically taken to be slice(None). So, it could be:
def _slice_at_axis(axis, slice):
return (slice(None),) * axis + (slice,)
(This is substantially faster than the loop, at least for a randomly selected axis=3.)
There was a problem hiding this comment.
Perhaps pass in slice(arr.shape[axis] - padding, None) in that case - at least it is immediately obvious what the code does.
There was a problem hiding this comment.
I was trying to avoid writing axis twice, hence the lambda function above.
This is substantially faster than the loop
I suspect this would be true if I kept my existing function names but changed their implementations too (I deliberately didn't do that in this PR, because it makes the diff clearer)
2d52077 to
7dd7938
Compare
|
Updated to build my helper functions out of the simpler helper function you propose (which is useful for a few other pieces of cleanup. Also renamed from |
e25e01b to
944f1b4
Compare
numpy/lib/arraypad.py
Outdated
There was a problem hiding this comment.
Yes, the last half is redundant - but it produces the same slices as we did before, and I see little reason to change that.
This makes `_slice_first` almost a factor of two faster
944f1b4 to
5608636
Compare
| end = arr.shape[axis] - 1 | ||
| if num is not None: | ||
| mean_slice = tuple( | ||
| slice(None) if i != axis else slice(end, end - num, -1) |
There was a problem hiding this comment.
Note that this slice is the reverse of what it was before - but min, max, mean, and med all are order-invariant anyway
mhvk
left a comment
There was a problem hiding this comment.
I'm a bit surprised by the double slicing, but that was there already and I think this is all OK now, so approved! Maybe squash the two commits, though?
|
I think it's clearer as is with two commits - extracting helper functions from the existing code, then rewriting those helpers to be more efficient |
|
OK, that's fine too. Will merge... |
|
Thanks for the feedback! |
Follows on from #11011