ENH: Add keepdims argument to count_nonzero#15870
Conversation
|
The TravisCI test failure on the s390x platform looks unrelated to this PR. |
|
Reran the tests and all passed--looks like the TravisCI test failure on the s390x platform was a transient. |
|
Needs a release note, but this looks like an obviously sane addition. |
|
Thanks @eric-wieser, I added a release note. |
seberg
left a comment
There was a problem hiding this comment.
LGTM, just the None path needs fixing. Nice generalization, I agree that this is a reduce-like operation and thus should have the keepdims argument. kwarg-only is a nice touch.
numpy/core/numeric.py
Outdated
|
|
||
|
|
||
| def _count_nonzero_dispatcher(a, axis=None): | ||
| def _count_nonzero_dispatcher(a, axis=None, *, keepdims=False): |
There was a problem hiding this comment.
@shoyer shouldn't this be always None for the dispatching signature, did it actually matter?
There was a problem hiding this comment.
I think it was just convention to avoid defaults getting out of sync. Still worthwhile though.
There was a problem hiding this comment.
OK, I changed the keepdims default in the dispatcher function signature to None.
There was a problem hiding this comment.
Hmm, it looks like our import-time check to verify default values of dispatcher arguments may be broken on keyword only argument. This should have caused a test failure.
numpy/core/numeric.py
Outdated
| array([[2], | ||
| [3]]) | ||
| """ | ||
| if axis is None: |
There was a problem hiding this comment.
This seems incorrect, it has to be axis is None and not keepdims. Also needs a test.
There was a problem hiding this comment.
Good catch, fixed.
|
Thanks, lets put this in! |
A simple enhancement to count_nonzero: