ENH: adds np.nancumsum and np.nancumprod #7421

pwolfram · 2016-03-16T17:35:13Z

This PR adds an implementation of nancumsum and nancumprod.

The actual function is a two-liner adapted from nansum and nanprod.

Its structure is adapted from PR: #5418 ( a minor typo in the doc string from this PR is fixed too)

pwolfram · 2016-03-16T17:35:36Z

Corresponds to PR numpy#7421, adpated from numpy#7410

shoyer · 2016-03-16T17:42:17Z

Just a note -- since this is new API, somebody might ask you to write the numpy-discussions mailing list. But I think this should be pretty uncontroversial 👍.

shoyer · 2016-03-16T17:43:59Z

Consistency checks with integer arrays are great, but these also need tests on arrays with NaNs.

pwolfram · 2016-03-16T17:52:01Z

Thanks @shoyer, I accidentally left that out. I updated this to include tests on arrays with NaNs following nanprod's lead. Does this work?

charris · 2016-03-16T21:02:04Z

Could you put the documentation commit in this PR as well? Note that you can keep adding commits to the PR, and later clean up the history with git rebase -i if needed.

pwolfram · 2016-03-16T22:03:46Z

@charris, done. @shoyer, I've also added the testing capability as you requested. Thanks!

shoyer · 2016-03-16T22:57:14Z

numpy/lib/nanfunctions.py

+
+    One is returned for slices that are all-NaN or empty.
+
+    .. versionadded:: 1.11.0


We're already at the release candidate stage for 1.11, so this will make it into 1.12

pwolfram · 2016-03-17T18:19:20Z

@charris and @shoyer, is there anything else that needs done on this PR?

seberg · 2016-03-17T22:51:13Z

numpy/lib/nanfunctions.py

+    Return the cumulative sum of array elements over a given axis treating Not a
+    Numbers (NaNs) as zero.
+
+    One is returned for slices that are all-NaN or empty.


I guess this is supposed to say zero?

@seberg, thanks for finding that typo (should have been plural form of verb too, not singular-- same also for nancumprod)

njsmith · 2016-03-17T23:11:52Z

Mailing list thread: https://mail.scipy.org/pipermail/numpy-discussion/2016-March/075169.html

madphysicist · 2016-03-18T01:25:37Z

numpy/lib/nanfunctions.py

@@ -548,7 +551,7 @@ def nanprod(a, axis=None, dtype=None, out=None, keepdims=np._NoValue):
    Parameters
    ----------
    a : array_like
-        Array containing numbers whose sum is desired. If `a` is not an
+        Array containing numbers whose product is desired. If `a` is not an
        array, a conversion is attempted.


While you're at it, nans are treated like "one", not "zero" on line 542/545.

Also, if I am not mistaken, axis, two lines below, accepts a tuple now. The docs should be updated according to np.prod.

Not for the .accumulate() method used by the cumxxx functions: only one axis.

My mistake. By the way, would it make sense to apply the _ureduce function from numpy.lib.function_base to other places, like most of numpy.core.from_numeric? That way we could add multi-axis support without waiting for the gufunc redesign that seems to be coming.

Which functions do you have in mind? I don't see any obvious place where _ureduce could help...

ptp, argmin, argmax. Also, the following might benefit as well, but the internal order of the array would matter: partition, argpartition, sort, argsort, searchsorted, cumsum, cumprod. I was even thinking that it might be worth allowing ravel to do a partial ravel along a subset of the axes (basically like _ureduce does), but that is probably going to require some major rewriting to do properly.

Thanks @madphysicist, change made by 8fd1ee8 fixes the typo.

Only that I made a mistake. Axis only accepts one value here. You were absolutely right.

madphysicist · 2016-03-18T01:40:59Z

numpy/lib/nanfunctions.py

+    Return the cumulative sum of array elements over a given axis treating Not a
+    Numbers (NaNs) as zero.
+
+    Zeros are returned for slices that are all-NaN or empty.


Mention that the sum does not change when nans are encountered and that any leading nans are replaced by zeros.

@madphysicist, thanks, should be fixed by 41adc0f

Have you pushed that commit yet?

madphysicist · 2016-03-18T01:52:48Z

Aside from the comments about docs, LGTM. And by all means feel free to ignore the comments about functions other than your own.

pwolfram · 2016-03-18T15:11:31Z

@madphysicist, I've pushed commits to address your comments (including for nansum and nanprod). Please let me know if you see any further room for improvement. Thanks!

pwolfram · 2016-03-18T15:12:33Z

@madphysicist, actually there are more following a page refresh... hold on a bit. I'll let you know when the others are pushed.

madphysicist · 2016-03-18T15:12:54Z

numpy/lib/nanfunctions.py

+    nansum_along_axis : ndarray.
+        A new array holding the result is returned unless `out` is
+        specified, in which it is returned. The
+        result has the same size as `a`, and the same shape as `a` if


Sorry to nitpick, but the line lengths look funny here. Also, _along_axis is inconsistent with the other functions. I think you should just leave it as nansum.

Agreed I wondered about that myself but didn't want to depart from convention. Fixed in 29cca2d

Also in 6ae90ec

pwolfram · 2016-03-24T02:47:59Z

Changes made, thanks for the keen attention to detail and for helping me improve the code @shoyer and @seberg!

shoyer · 2016-03-24T03:55:23Z

Reading PEP8 more carefully, it looks like I'm wrong about spacing around binary operators:
https://www.python.org/dev/peps/pep-0008/#other-recommendations

Hmm....

pwolfram · 2016-03-24T14:06:33Z

@shoyer, reverted spacing back on * because I don't think this makes the code easier to read but left it for % because it adds clarity in accordance with the pep8 guidelines.

shoyer · 2016-03-24T17:29:20Z

numpy/lib/tests/test_nanfunctions.py

@@ -22,6 +22,18 @@
         np.array([0.1042, -0.5954]),
         np.array([0.1610, 0.1859, 0.3146])]

+# Rows of _ndat with nans converted to ones
+_rdat_ones = [np.array([0.6244, 1.0, 0.2692, 0.0116, 1.0, 0.1170]),


I think this should probably be all one big array -- it's only not one array for _rdat because _rdat is ragged

shoyer · 2016-03-24T20:41:52Z

@pwolfram I think this is very close, though a test for negative axes would be nice. Otherwise this looks good to me. Sorry for leading your astray on pep8!

pwolfram · 2016-03-24T20:59:42Z

numpy/lib/tests/test_nanfunctions.py

+            assert_almost_equal(res, tgt)
+            tgt = np.cumsum(_ndat_zeros,axis=axis)
+            res = np.nancumsum(_ndat, axis=axis)
+            assert_almost_equal(res, tgt)


@shoyer, was this what you were thinking?

yes, exactly

This PR adds an implementation of `nancumsum` and `nancumprod`. The actual function is a two-liner adapted from `nansum`. Its structure is adapted from PR: numpy#5418

pwolfram · 2016-03-24T21:09:30Z

numpy/lib/tests/test_nanfunctions.py

+                res = nf(mat, axis=axis, out=resout)
+                assert_almost_equal(res, resout)
+                assert_almost_equal(res, tgt)
+


@shoyer, I also generalized the test here for consistency and greater coverage.

pwolfram · 2016-03-24T21:10:30Z

@shoyer, please see above tests added over the negative axis. Thanks for helping refine this!

pwolfram · 2016-03-24T21:12:47Z

Also, btw @shoyer, no worries about the pep8 confusion. I installed the plugin for vim and I have a clearer idea of formatting standards so it was certainly value-added!

shoyer · 2016-03-24T21:17:53Z

OK, this looks good to me. I'll merge this once tests pass unless anyone speaks up with objections...

pwolfram · 2016-03-26T18:20:19Z

Thanks @shoyer!

Needed until numpy v1.12, see numpy/numpy#7421

* Adds nancumsum, nancumprod for numpy compatability Needed until numpy v1.12, see numpy/numpy#7421 * Adds nancumsum, nancumprod to xarray functions

pwolfram added a commit to pwolfram/numpy that referenced this pull request Mar 16, 2016

DOC: add nancumprod/nancumsum to math routine list

12f17cf

Corresponds to PR numpy#7421, adpated from numpy#7410

pwolfram mentioned this pull request Mar 16, 2016

DOC: add nancumprod/nancumsum to math routine list #7422

Closed

pwolfram force-pushed the nancumsumprod branch from 42994d3 to 1a23388 Compare March 16, 2016 17:49

pwolfram force-pushed the nancumsumprod branch from 1a23388 to 91b627e Compare March 16, 2016 22:02

pwolfram force-pushed the nancumsumprod branch from 91b627e to 8903b5c Compare March 16, 2016 22:03

shoyer reviewed Mar 16, 2016
View reviewed changes

seberg reviewed Mar 17, 2016
View reviewed changes

pwolfram force-pushed the nancumsumprod branch from 564b9fd to ecda4b1 Compare March 17, 2016 23:02

charris added 01 - Enhancement component: numpy.lib labels Mar 17, 2016

madphysicist reviewed Mar 18, 2016
View reviewed changes

madphysicist mentioned this pull request Mar 18, 2016

ENH Generalized rot90 #7347

Merged

madphysicist reviewed Mar 18, 2016
View reviewed changes

pwolfram force-pushed the nancumsumprod branch from 75d6df6 to a212a88 Compare March 24, 2016 02:46

pwolfram force-pushed the nancumsumprod branch from a212a88 to 85084d1 Compare March 24, 2016 14:05

shoyer reviewed Mar 24, 2016
View reviewed changes

pwolfram force-pushed the nancumsumprod branch from 85084d1 to 74ca05b Compare March 24, 2016 20:57

pwolfram reviewed Mar 24, 2016
View reviewed changes

ENH: adds np.nancumsum and np.nancumprod

a76b872

This PR adds an implementation of `nancumsum` and `nancumprod`. The actual function is a two-liner adapted from `nansum`. Its structure is adapted from PR: numpy#5418

pwolfram force-pushed the nancumsumprod branch from 74ca05b to a76b872 Compare March 24, 2016 21:09

pwolfram reviewed Mar 24, 2016
View reviewed changes

shoyer added this to the 1.12.0 release milestone Mar 26, 2016

shoyer merged commit ef389ee into numpy:master Mar 26, 2016

This was referenced Mar 26, 2016

ENH: added functionality nancov to numpy #5698

Closed

TST: Suppressed warnings #7099

Merged

pwolfram added a commit to pwolfram/xarray that referenced this pull request Mar 28, 2016

Adds nancumsum, nancumprod

c0172e2

Needed until numpy v1.12, see numpy/numpy#7421

pwolfram mentioned this pull request Mar 31, 2016

Adds cummulative operators to API pydata/xarray#812

Merged

2 tasks

pwolfram added a commit to pwolfram/xarray that referenced this pull request Sep 20, 2016

Adds nancumsum, nancumprod

74ece06

Needed until numpy v1.12, see numpy/numpy#7421

pwolfram added a commit to pwolfram/xarray that referenced this pull request Sep 20, 2016

Adds nancumsum, nancumprod for numpy compatability

de8b77f

Needed until numpy v1.12, see numpy/numpy#7421

pwolfram added a commit to pwolfram/xarray that referenced this pull request Sep 20, 2016

Adds nancumsum, nancumprod

c6ea006

Needed until numpy v1.12, see numpy/numpy#7421

pwolfram added a commit to pwolfram/xarray that referenced this pull request Sep 20, 2016

Adds nancumsum, nancumprod for numpy compatability

6174efb

Needed until numpy v1.12, see numpy/numpy#7421

pwolfram added a commit to pwolfram/xarray that referenced this pull request Sep 20, 2016

Adds nancumsum, nancumprod for numpy compatability

6611002

Needed until numpy v1.12, see numpy/numpy#7421

pwolfram added a commit to pwolfram/xarray that referenced this pull request Oct 3, 2016

Adds nancumsum, nancumprod for numpy compatability

428d859

Needed until numpy v1.12, see numpy/numpy#7421

shoyer pushed a commit to pydata/xarray that referenced this pull request Oct 3, 2016

Adds cummulative operators to API (#812)

9cf107b

* Adds nancumsum, nancumprod for numpy compatability Needed until numpy v1.12, see numpy/numpy#7421 * Adds nancumsum, nancumprod to xarray functions


		One is returned for slices that are all-NaN or empty.

		.. versionadded:: 1.11.0

ENH: adds np.nancumsum and np.nancumprod #7421

ENH: adds np.nancumsum and np.nancumprod #7421

Conversation

pwolfram commented Mar 16, 2016

pwolfram commented Mar 16, 2016

shoyer commented Mar 16, 2016

shoyer commented Mar 16, 2016

pwolfram commented Mar 16, 2016

charris commented Mar 16, 2016

pwolfram commented Mar 16, 2016

Choose a reason for hiding this comment

pwolfram commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madphysicist commented Mar 18, 2016

pwolfram commented Mar 18, 2016

pwolfram commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwolfram commented Mar 24, 2016

shoyer commented Mar 24, 2016

pwolfram commented Mar 24, 2016

Choose a reason for hiding this comment

shoyer commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwolfram commented Mar 24, 2016

pwolfram commented Mar 24, 2016

shoyer commented Mar 24, 2016

pwolfram commented Mar 26, 2016