-
Notifications
You must be signed in to change notification settings - Fork 50
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 specification for computing the cumulative sum #653
Conversation
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.
Thanks @kgryte!
My initial comments:
-
I look at
cumulative_sum
like a reduction, which should closely follow whatsum
and other reductions do for its main parameters. -
The use cases about "prepending a value" are tricky because of the output becoming keyword-dependent. As I read it, that is what both
startpoint
andendpoint
do, right?- It kinda breaks our policy regarding output shapes depending on keywords, so I think we need an explicit okay for this from JAX & co.
- Most of the discussion was specifically about prepending zero, which is
startpoint
. I'm less convinced by the inclusion ofendpoint
.
-
initial
is indeed useful to leave out here, because if we want to add it we should do that uniformly for all reductions.
startpoint: bool | ||
boolean indicating whether to include the initial value as the first value in the output. Default: ``False``. | ||
endpoint: bool | ||
boolean indicating whether to include the total sum as the last value in the output. Default: ``True``. |
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.
I will note that initial=
would be a useful extension to allow starting with something other than 0, especially together with startpoint=True
. (It might even be necessary in case an identity isn't defined: such as for timedelta's were there startpoint has to be a datetime.)
Mainly mentioning it in case anyone is inspired about startpoint/endpoint naming, I don't mind adding such parameters (not sure how hard it is to add to numpy, but I don't expect it is hard).
Is there a reason this PR stalled? I think cumulative sums are quite pervasive and would be a good candidate for standardisation. |
No reason beyond limited bandwidth. This function will definitely make it into the next release. |
Update: I added guidance concerning what happens when |
Update: based on feedback during Consortium discussions, I've updated the guidance concerning what should happen when The guidance now states that, no matter the axis size (>=0), the output array shape must follow the same rule: namely, the size of the axis along which to compute the cumulative sum must be This change ensures that the output array shape follows consistent rules and is not dependent on the input array shape. |
@seberg Would you be willing to do another round of review? Would be good to get another set of eyes on this before we merge. Thanks! |
FWIW, I am happy with |
As this PR has been discussed previously and no objections were raised, will go ahead and merge. Any further updates or clarifications can be addressed in follow-ups PRs. |
This PR
cumulative_sum
. This follows recent convention in the dataframe API standard (ref: Add some cumulative Column operations dataframe-api#193).dtype
conventions established in the specifications forsum
, whereby, if nodtype
is specified, the data type of the output array must have a range of values at least as large as the default data type of the same kind.axis
conventions whereby, for one-dimensional arrays, providing anaxis
is optional; however, if the input array has more than one dimension, providing anaxis
is required. This deviates from NumPy where not providing anaxis
computes a cumulative sum over the flattened array and follows PyTorch where providing an axis is required. The solution here attempts to thread the needle between the two, siding with convention established intake
where convenience is given greater weight for one-dimensional arrays.include_initial
keyword argument to control whether the initial value (by convention, the additive identity) is included in the output. Theinclude_initial
naming convention takes inspiration from NumPy#6044 which proposedinclude_identity
, but this PR uses_initial
to align with a future extension in which aninitial
kwarg specifies a custom initial value. By default,include_initial=False
, which matches the default behavior across the ecosystem. As of this PR, only TensorFlow supports excluding/including the starting summation value.NaN
and infinities) must follow special cases as defined inadd
.Questions
axis
be fully optional, as in NumPy?take
convention, where NumPy's flattening behavior is not supported.startpoint
andendpoint
? Or are we okay following specification precedent?endpoint
, especially whenendpoint=False
can be achieved by slicing off the last element. Omission of the last element is a common use case (e.g., when computing offsets), but it's explicit support in the API signature did not seem warranted. Further, concerns were raised regardingstartpoint
/endpoint
naming conventions in that they lacked obviousness. This PR has been updated to useinclude_initial
with potential future alignment with future extension in whichinitial
kwarg support is added tocumulative_sum
.