fix: keepdims in numpy.sum should not be None

[Saransh-cpp]

Description

Fixes #372

See #372 (comment)

I've added tests for the default case as well.

Checklist

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't any other open Pull Requests for the required change?
• Does your submission pass pre-commit? ($ pre-commit run --all-files or $ nox -s lint)
• Does your submission pass tests? ($ pytest or $ nox -s tests)
• Does the documentation build with your changes? ($ cd docs; make clean; make html or $ nox -s docs)
• Does your submission pass the doctests? ($ xdoctest ./src/vector or $ nox -s doctests)

Before Merging

• Summarize the commit messages into a brief review of the Pull request.

[Saransh-cpp]

I'm not sure if there is a public API for numpy._NoValue? Should we be using the private API or is there a better way to do this? Or should I set keepdims=False by default?

[jpivarski]

Set keepdims=False by default.

Suggested change

from numpy._globals import _NoValue, _NoValueType

Anything that starts with an underscore is not safe for other libraries to use, so that would include this one.

NumPy's default value for keepdims is effectively False; that's what's used if no keepdims argument is given.

>>> np.sum(np.ones((3, 5)), axis=1)
array([5., 5., 5.])
>>> np.sum(np.ones((3, 5)), axis=1, keepdims=False)
array([5., 5., 5.])
>>> np.sum(np.ones((3, 5)), axis=1, keepdims=True)
array([[5.],
       [5.],
       [5.]])

They have this _NoValue to distinguish "user did not specify a keepdims value" from "user explicitly specified the value that would be the default, anyway." The reason they're making that distinction is given in the np.sum documentation:

If the default value is passed, then keepdims will not be passed through to the sum method of sub-classes of ndarray, however any non-default value will be. If the sub-class’ method does not implement keepdims any exceptions will be raised.

presumably to allow for subclasses that did not handle this argument, maybe because they predated it?

If Vector just sets the default keepdims=False, then it would not carry over this behavior to subclasses of vector.backends.numpy.VectorNumpy*D, but that's probably fine. We can just declare that while NumPy doesn't expect subclasses of np.ndarray to have a keepdims argument on its sum method, we do expect subclasses of vector.backends.numpy.VectorNumpy*D to have such an argument. An inheritance subtree can make stronger assumptions that parents of that subtree.

If this is also the case for other reducers, then this PR should probably fix them as well, taking keepdims=False as the default everywhere.

Note: this is how Awkward Array solves the same problem:

https://github.com/scikit-hep/awkward/blob/684a84deffb51c75ba05ec3ca69ad964c3ff5845/src/awkward/operations/ak_sum.py#L15-L23

It handles every argument that NumPy handles, but it does have stronger constraints on them. (Now that I'm looking at it, we seem to be missing initial...)

[Saransh-cpp]

They have this _NoValue to distinguish "user did not specify a keepdims value" from "user explicitly specified the value that would be the default, anyway." The reason they're making that distinction is given in the np.sum documentation:

Oh, that makes sense, I missed this.

Thanks for the detailed explanation! This was very helpful!

[jpivarski]

Ah, so it wasn't a missing implementation of the whole function, just one argument.

Does Vector implement any other reducers? If so, the same mistake might be made there, too.

More inline below. (Technically, I'm requesting changes, but they're suggestions that can just be accepted—no need to redo the review afterward.)

[jpivarski]

Set keepdims=False by default.

Suggested change

from numpy._globals import _NoValue, _NoValueType

Anything that starts with an underscore is not safe for other libraries to use, so that would include this one.

NumPy's default value for keepdims is effectively False; that's what's used if no keepdims argument is given.

>>> np.sum(np.ones((3, 5)), axis=1)
array([5., 5., 5.])
>>> np.sum(np.ones((3, 5)), axis=1, keepdims=False)
array([5., 5., 5.])
>>> np.sum(np.ones((3, 5)), axis=1, keepdims=True)
array([[5.],
       [5.],
       [5.]])

They have this _NoValue to distinguish "user did not specify a keepdims value" from "user explicitly specified the value that would be the default, anyway." The reason they're making that distinction is given in the np.sum documentation:

If the default value is passed, then keepdims will not be passed through to the sum method of sub-classes of ndarray, however any non-default value will be. If the sub-class’ method does not implement keepdims any exceptions will be raised.

presumably to allow for subclasses that did not handle this argument, maybe because they predated it?

If Vector just sets the default keepdims=False, then it would not carry over this behavior to subclasses of vector.backends.numpy.VectorNumpy*D, but that's probably fine. We can just declare that while NumPy doesn't expect subclasses of np.ndarray to have a keepdims argument on its sum method, we do expect subclasses of vector.backends.numpy.VectorNumpy*D to have such an argument. An inheritance subtree can make stronger assumptions that parents of that subtree.

If this is also the case for other reducers, then this PR should probably fix them as well, taking keepdims=False as the default everywhere.

Note: this is how Awkward Array solves the same problem:

https://github.com/scikit-hep/awkward/blob/684a84deffb51c75ba05ec3ca69ad964c3ff5845/src/awkward/operations/ak_sum.py#L15-L23

It handles every argument that NumPy handles, but it does have stronger constraints on them. (Now that I'm looking at it, we seem to be missing initial...)

[Saransh-cpp]

Thanks for the review!

Does Vector implement any other reducers? If so, the same mistake might be made there, too.

Yes, just double checked, none of them makes the same mistake.

I'll create a quick bug-fix release after merging this PR.