-
-
Notifications
You must be signed in to change notification settings - Fork 820
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
Add apply_along_axis
#4008
Add apply_along_axis
#4008
Conversation
In most cases, for ufuncs that take an axis argument it will be preferable to use that rather than this function. However, here is a demo of a case where reduction along a non-contiguous axis via import cupy as cp
from cupyx.time import repeat
a = cp.random.randn(1000000, 16)
perf1 = repeat(cp.mean, (a, 0), n_warmup=20, n_repeat=20)
perf2 = repeat(cp.apply_along_axis, (cp.mean, 0, a), n_warmup=20, n_repeat=20)
print("Result for cp.mean:")
print(perf1)
print("Result for cp.apply_along_axis:")
print(perf2)
On the contrary, if I create a with shape (16, 1000000) and take the reduction along axis 1 (contiguous), the computation is much faster and the built-in a = cp.random.randn(16, 1000000)
perf1 = repeat(cp.mean, (a, 1), n_warmup=20, n_repeat=20)
perf2 = repeat(cp.apply_along_axis, (cp.mean, 1, a), n_warmup=20, n_repeat=20)
print("Result for cp.mean:")
print(perf1)
print("Result for cp.apply_along_axis:")
print(perf2)
|
If the new scalar moveaxis function is not desired let me know and I will refactor to only include the minor Cython improvements to the existing moveaxis function. This PR reduces the time for a call like |
The |
Just my two cents, you should wait for @asi1024's reply: The helper should be moved to |
That seems like a reasonable location. If we do that then the existing The other style I see is in cupy/cupy/core/_routines_sorting.pyx Lines 34 to 37 in dd2fb09
I am not sure if that helps avoid any function call overhead or if those should also just call this utility. A couple of other places in the I think my preference would be to move to internal as you suggest and just use this version for everything within core. |
cupy/lib/shape_base.py
Outdated
'Cannot apply_along_axis when any iteration dimensions are 0' | ||
) | ||
# cupy.asarray needed in case func1d returns a scalar | ||
res = cupy.asarray(func1d(inarr_view[ind0], *args, **kwargs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If func1d
returns a numpy.ndarray
, it will be converted to cupy.ndarray in this line. However, ValueError
will be raised in L64 only if the length of the reduction axis is greater than 1.
How about checking if the return value of func1d
is scalar or cupy.ndarray value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if the change in 3f6ec11 matches what your were suggesting? Now if func1d
returns a non-scalar numpy array, a value ValueError ("non-scalar numpy.ndarray cannot be used for fill"
) will occur when trying to assign the NumPy result to buff
.
I agree to move |
DOC: add apply_along_axis to the API docs
improve efficiency of _routines_manipulation.moveaxis by avoiding repeated acces of a.ndim TST: add tests for invalid scalar inputs to moveaxis
e968b55
to
3f6ec11
Compare
Jenkins, test this please. |
Jenkins CI test (for commit 3f6ec11, target branch master) succeeded! |
LGTM! |
This PR implements
apply_along_axis
which is a utility to repeatedly apply a 1d function along a given axis, looping over all other axes.This function is slightly simplified from NumPy's because it doesn't support a separate
matrix
class and doesn't try to preserve array subclasses likenumpy.ma.core.MaskedArray
via__array_wrap__
(neither masked arrays or__array_wrap__
are currently implemented by CuPy)