Re: [Numpy-discussion] axis parameter for count_nonzero
![](https://secure.gravatar.com/avatar/d495a692ec174d3053818ff85b96fbb7.jpg?s=120&d=mm&r=g)
Hi, I have had a PR <https://github.com/numpy/numpy/pull/7177> open (first draft can be found here <https://github.com/numpy/numpy/pull/7138>) for quite some time now that adds an 'axis' parameter to *count_nonzero*. While the functionality is fully in-place, very robust, and actually higher-performing than the original *count_nonzero* function, the obstacle at this point is the implementation, as most of the functionality is now surfaced at the Python level instead of at the C level. I have made several attempts to move the code into C to no avail and have not received much feedback from maintainers unfortunately to move this forward, so I'm opening this up to the mailing list to see what you guys think of the changes and whether or not it should be merged in as is or be tabled until a more C-friendly solution can be found. Thanks!
![](https://secure.gravatar.com/avatar/5f88830d19f9c83e2ddfd913496c5025.jpg?s=120&d=mm&r=g)
On Sun, May 22, 2016 at 3:05 AM, G Young <gfyoung17@gmail.com> wrote:
The discussion is spread over several PRs/issues, so maybe a summary is useful: - adding an axis parameter was a feature request that was generally approved of [1] - writing the axis selection/validation code in C, like the rest of count_nonzero, was preferred by several core devs - Writing that C code turns out to be tricky. Jaime had a PR for doing this for bincount [2], but closed it with final conclusion "the proper approach seems to me to build some intermediate layer over nditer that abstracts the complexity away". - Julian pointed out that this adds a ufunc-like param, so why not add other params like out/keepdims [3] - Stephan points out that the current PR has quite a few branches, would benefit from reusing a helper function (like _validate_axis, but that may not do exactly the right thing), and that he doesn't want to merge it as is without further input from other devs [4]. Points previously not raised that I can think of: - count_nonzero is also in the C API [5], the axis parameter is now only added to the Python API. - Part of why the code in this PR is complex is to keep performance for small arrays OK, but there's no benchmarks added or result given for the existing benchmark [6]. A simple check with: x = np.arange(100) %timeit np.count_nonzero(x) shows that that gets about 30x slower (330 ns vs 10.5 us on my machine). It looks to me like performance is a concern, and if that can be resolved there's the broader discussion of whether it's a good idea to merge this PR at all. That's a trade-off of adding a useful feature vs. technical debt / maintenance burden plus divergence Python/C API. Also, what do we do when we merge this and then next week someone else sends a PR adding a keepdims or out keyword? For these kinds of additions it would feel better if we were sure that the new version is the final/desired one for the foreseeable future. Ralf [1] https://github.com/numpy/numpy/issues/391 [2] https://github.com/numpy/numpy/pull/4330#issuecomment-77791250 [3] https://github.com/numpy/numpy/pull/7138#issuecomment-177202894 [4] https://github.com/numpy/numpy/pull/7177 [5] http://docs.scipy.org/doc/numpy/reference/c-api.array.html#c.PyArray_CountNo... [6] https://github.com/numpy/numpy/blob/master/benchmarks/benchmarks/bench_ufunc...
![](https://secure.gravatar.com/avatar/d495a692ec174d3053818ff85b96fbb7.jpg?s=120&d=mm&r=g)
1) Correction: The PR was not written with small arrays in mind. I ran some new timing tests, and it does perform worse on smaller arrays but appears to scale better than the current implementation. 2) Let me put it out there that I am not opposed to moving it to C, but right now, there seems to be a large technical brick wall up against such an implementation. So suggestions about how to move the code into C would be welcome too! On Sun, May 22, 2016 at 10:32 AM, Ralf Gommers <ralf.gommers@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/d495a692ec174d3053818ff85b96fbb7.jpg?s=120&d=mm&r=g)
After some discussion with *@rgommers*, I have simplified the code as follows: 1) the path to the original count_nonzero in the C API is essentially unchanged, save some small overhead with Python calling and the if-statement to check the *axis* parameter 2) All of the complicated validation of the *axis* parameter and acrobatics for getting the count is handled *only* after we cannot fast-track via a numerical, boolean, or string *dtype*. The question still remains whether or not leaving the *axis* parameter in the Python API for now (given how complicated it is to add in the C API) is acceptable. I will say that in response to the concern of adding parameters such as "out" and "keepdims" (should they be requested), we could avail ourselves to functions like median <https://github.com/numpy/numpy/blob/master/numpy/lib/function_base.py#L3528> for help as *@juliantaylor* pointed out. The *scipy* library has dealt with this problem as well in its *sparse* modules, so that is also a useful resource. On Sun, May 22, 2016 at 1:35 PM, G Young <gfyoung17@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/d495a692ec174d3053818ff85b96fbb7.jpg?s=120&d=mm&r=g)
Just wanted to ping the mailing this again about this PR. Just to recap, some simplification has been done thanks to some suggestions by *@rgommers*, though the question still remains whether or not leaving the *axis* parameter in the Python API for now (given how complicated it is to add in the C API) is acceptable. I will say that in response to the concern of adding parameters such as "out" and "keepdims" (should they be requested), we could avail ourselves to functions like median <https://github.com/numpy/numpy/blob/master/numpy/lib/function_base.py#L3528> for help as *@juliantaylor* pointed out. The *scipy* library has dealt with this problem as well in its *sparse* modules, so that is also a useful resource. Feedback on this issue would be much appreciated! Thanks! On Sun, May 22, 2016 at 1:36 PM, G Young <gfyoung17@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/5f88830d19f9c83e2ddfd913496c5025.jpg?s=120&d=mm&r=g)
On Sun, May 22, 2016 at 3:05 AM, G Young <gfyoung17@gmail.com> wrote:
The discussion is spread over several PRs/issues, so maybe a summary is useful: - adding an axis parameter was a feature request that was generally approved of [1] - writing the axis selection/validation code in C, like the rest of count_nonzero, was preferred by several core devs - Writing that C code turns out to be tricky. Jaime had a PR for doing this for bincount [2], but closed it with final conclusion "the proper approach seems to me to build some intermediate layer over nditer that abstracts the complexity away". - Julian pointed out that this adds a ufunc-like param, so why not add other params like out/keepdims [3] - Stephan points out that the current PR has quite a few branches, would benefit from reusing a helper function (like _validate_axis, but that may not do exactly the right thing), and that he doesn't want to merge it as is without further input from other devs [4]. Points previously not raised that I can think of: - count_nonzero is also in the C API [5], the axis parameter is now only added to the Python API. - Part of why the code in this PR is complex is to keep performance for small arrays OK, but there's no benchmarks added or result given for the existing benchmark [6]. A simple check with: x = np.arange(100) %timeit np.count_nonzero(x) shows that that gets about 30x slower (330 ns vs 10.5 us on my machine). It looks to me like performance is a concern, and if that can be resolved there's the broader discussion of whether it's a good idea to merge this PR at all. That's a trade-off of adding a useful feature vs. technical debt / maintenance burden plus divergence Python/C API. Also, what do we do when we merge this and then next week someone else sends a PR adding a keepdims or out keyword? For these kinds of additions it would feel better if we were sure that the new version is the final/desired one for the foreseeable future. Ralf [1] https://github.com/numpy/numpy/issues/391 [2] https://github.com/numpy/numpy/pull/4330#issuecomment-77791250 [3] https://github.com/numpy/numpy/pull/7138#issuecomment-177202894 [4] https://github.com/numpy/numpy/pull/7177 [5] http://docs.scipy.org/doc/numpy/reference/c-api.array.html#c.PyArray_CountNo... [6] https://github.com/numpy/numpy/blob/master/benchmarks/benchmarks/bench_ufunc...
![](https://secure.gravatar.com/avatar/d495a692ec174d3053818ff85b96fbb7.jpg?s=120&d=mm&r=g)
1) Correction: The PR was not written with small arrays in mind. I ran some new timing tests, and it does perform worse on smaller arrays but appears to scale better than the current implementation. 2) Let me put it out there that I am not opposed to moving it to C, but right now, there seems to be a large technical brick wall up against such an implementation. So suggestions about how to move the code into C would be welcome too! On Sun, May 22, 2016 at 10:32 AM, Ralf Gommers <ralf.gommers@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/d495a692ec174d3053818ff85b96fbb7.jpg?s=120&d=mm&r=g)
After some discussion with *@rgommers*, I have simplified the code as follows: 1) the path to the original count_nonzero in the C API is essentially unchanged, save some small overhead with Python calling and the if-statement to check the *axis* parameter 2) All of the complicated validation of the *axis* parameter and acrobatics for getting the count is handled *only* after we cannot fast-track via a numerical, boolean, or string *dtype*. The question still remains whether or not leaving the *axis* parameter in the Python API for now (given how complicated it is to add in the C API) is acceptable. I will say that in response to the concern of adding parameters such as "out" and "keepdims" (should they be requested), we could avail ourselves to functions like median <https://github.com/numpy/numpy/blob/master/numpy/lib/function_base.py#L3528> for help as *@juliantaylor* pointed out. The *scipy* library has dealt with this problem as well in its *sparse* modules, so that is also a useful resource. On Sun, May 22, 2016 at 1:35 PM, G Young <gfyoung17@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/d495a692ec174d3053818ff85b96fbb7.jpg?s=120&d=mm&r=g)
Just wanted to ping the mailing this again about this PR. Just to recap, some simplification has been done thanks to some suggestions by *@rgommers*, though the question still remains whether or not leaving the *axis* parameter in the Python API for now (given how complicated it is to add in the C API) is acceptable. I will say that in response to the concern of adding parameters such as "out" and "keepdims" (should they be requested), we could avail ourselves to functions like median <https://github.com/numpy/numpy/blob/master/numpy/lib/function_base.py#L3528> for help as *@juliantaylor* pointed out. The *scipy* library has dealt with this problem as well in its *sparse* modules, so that is also a useful resource. Feedback on this issue would be much appreciated! Thanks! On Sun, May 22, 2016 at 1:36 PM, G Young <gfyoung17@gmail.com> wrote:
participants (2)
-
G Young
-
Ralf Gommers