BUG: fix ma.diff not preserving mask when using append/prepend by markopacak · Pull Request #22776 · numpy/numpy

markopacak · 2022-12-12T12:52:13Z

Ported CORE diff to MA

Kept only relevant append/prepend code
Internally call CORE diff
docstring mentions mask preservation + change return type to MaskedArray
docstring examples adapted to MA module

Added 2 unit tests

seberg · 2022-12-20T12:37:49Z

Thanks, tests could probably be expanded, but seem OK. The code and examples are very close to the original, so that looks all fine.

The one thing I would like is to not define the method MaskedArray.diff and just make it a function.
We also don't have ndarray.diff (or try to call it from `np.diff) so I don't see a reason to add it here.

seberg · 2022-12-22T17:08:02Z

numpy/ma/core.py

 size.__doc__ = np.size.__doc__


+def diff(a, n=1, axis=-1, prepend=np._NoValue, append=np._NoValue):


Suggested change

def diff(a, n=1, axis=-1, prepend=np._NoValue, append=np._NoValue):

def diff(a, /, n=1, axis=-1, prepend=np._NoValue, append=np._NoValue):

Just thought we should start doing this, its not like a is a name that is nice enough we should promise it in all eternity.

I forgot one other thing (sorry about that): Can you add this function to the stubs file in core.pyi (should be able to adapt the original .pyi file again).

I wrote it in the form

def diff(a, /, n=..., axis=..., prepend=..., append=...): ...

like the rest of the file.

Let me know if I should write it like the original:

@overload def diff( a: _T, n: L[0], axis: SupportsIndex = ..., prepend: ArrayLike = ..., append: ArrayLike = ..., ) -> _T: ...

seberg

Sorry, too late to do a full review now, but wanted to comment on the doc changes.

numpy/ma/core.py

seberg · 2022-12-22T17:10:01Z

numpy/ma/core.py

+        arrays with length 1 in the direction of axis and the shape
+        of the input array in along all other axes.  Otherwise the
+        dimension and shape must match `a` except along axis.
+        .. versionadded:: 1.16.0


Also here newlines... You can remove the .. versionadded:: here, but it actually would be good to add .. versionadded:: 1.25.0 to the top (right before the Parameters section)!

markopacak · 2022-12-28T08:46:51Z

@seberg I have made a bit of a mess with accidentally committing some changes and then (kind of) revert (see 2 rev commits). Should I try to rewrite the history?

charris · 2022-12-28T14:01:50Z

It's fine to rewrite the history of pull requests.

Update numpy/ma/core.py Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

markopacak · 2022-12-31T18:00:03Z

I don't understand the linux_aarch64 test failing. Can anyone help me out?

charris · 2022-12-31T19:03:45Z

I don't understand the linux_aarch64 test failing

Network problem, it happens. I reran it.

numpy/ma/core.py

numpy/ma/core.pyi

seberg

Thanks, not sure we need the release note, but I guess it doesn't hurt.

@markopacak I am happy to put this in soon, because I think it looks correct.

Looking at the original code now, if n == 0 it returns a unmodified, but here we do not have that code branch. To be clear, the original code looks buggy here when prepend/append is used (I also don't like that a is returned directly rather than a copy in all other cases, but so be it...)

A test that it fails if a is 0-dimensional would also not be bad (I am sure it must fail in the concatenation, which was explicit in the np.diff code).

markopacak · 2023-01-08T10:08:43Z

@seberg Update: added check for a.ndim == 0 and for for n == 0 (just like in np.diff).

Note: I just realized that if I pass a non-MA array without prepend/append to MA diff, I get back a non-MA array, despite my docstring saying that a MaskedArray is always returned (I even added an example in the docstrings):

>>> u8_arr = np.array([1, 0], dtype=np.uint8)  # base np array
>>> np.ma.diff(u8_arr)  # MA diff
array([255], dtype=uint8)  # <-- base array returned, not a MaskedArray

Should I convert any input a to a MaskedArray (by using np.ma.asanyarray, the base diff does something similar)? The line would be placed right before if n == 0: ...

Or should I just add a note to the docs?

charris · 2023-01-17T22:22:03Z

@seberg Ping.

seberg · 2023-01-18T17:31:49Z

numpy/ma/core.py

+    if n < 0:
+        raise ValueError("order must be non-negative but got " + repr(n))
+
+    if a.ndim == 0:


Sorry for not looking earlier. I am happy to do this to keep it identical to the original code (will open an issue about it, I don't think n == 0 is correct, it should check for prepend/append to be None).

Doing a.ndim == 0 may fail on e.g. list inputs when it currently didn't fail, so we probably also need that np.ma.asanyarray() call first that the old function has.

Added np.ma.asanyarray to keep it identical to BASE diff.

Pls ping me when you open that issue, I'd also like to take a look.

seberg · 2023-01-18T23:39:34Z

Thanks, lets put it in, sorry it took a bit! CircleCI is failing because the PR was opened a bit ago and its now still on Python 3.8 here. It was working before and the docs weren't changed, so going to take that chance.

markopacak added 2 commits December 12, 2022 12:27

BUG: port CORE diff relevant code to MA and adapt docstrings examples

743257f

TST: add ma.diff tests

cfd6659

markopacak changed the title ~~Bug/ma diff prepend append~~ BUG: fix ma.diff not preserving mask when using append/prepend Dec 12, 2022

BUG: dont concatenate if no prepend/append are passed

715b54d

markopacak marked this pull request as ready for review December 12, 2022 16:54

seberg added 01 - Enhancement component: numpy.ma masked arrays 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. labels Dec 20, 2022

seberg reviewed Dec 22, 2022

View reviewed changes

markopacak marked this pull request as draft December 25, 2022 18:33

markopacak marked this pull request as ready for review December 26, 2022 18:18

markopacak and others added 4 commits December 29, 2022 16:34

DOC: add release/upcoming_changes

ba6f6fe

MAINT: move diff function outside MaskedArray class

4452833

Update numpy/ma/core.py Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

DOC: fix docstrings examples

da7aab6

DEV: add pyi

95aac56

markopacak force-pushed the bug/ma-diff-prepend-append branch from 7e86a5c to 95aac56 Compare December 29, 2022 16:44

seberg reviewed Jan 5, 2023

View reviewed changes

numpy/ma/core.py Outdated Show resolved Hide resolved

Update numpy/ma/core.py

67f0694

seberg reviewed Jan 5, 2023

View reviewed changes

numpy/ma/core.pyi Show resolved Hide resolved

seberg approved these changes Jan 5, 2023

View reviewed changes

markopacak added 2 commits January 15, 2023 19:18

ENH: check for n=0 and dim=0

f9bcaa7

STY: lines too long

baef40c

seberg reviewed Jan 18, 2023

View reviewed changes

ENH: convert input with ma.asanyarray

482e326

seberg merged commit 720cabc into numpy:main Jan 18, 2023

greglucas mentioned this pull request Aug 2, 2023

ENH: Add __array_function__ implementation to MaskedArray #22913

Open

		size.__doc__ = np.size.__doc__


		def diff(a, n=1, axis=-1, prepend=np._NoValue, append=np._NoValue):

	def diff(a, n=1, axis=-1, prepend=np._NoValue, append=np._NoValue):
	def diff(a, /, n=1, axis=-1, prepend=np._NoValue, append=np._NoValue):

Uh oh!

Conversation

markopacak commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Dec 20, 2022

Uh oh!

seberg Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

markopacak Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seberg Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

markopacak commented Dec 28, 2022

Uh oh!

charris commented Dec 28, 2022

Uh oh!

markopacak commented Dec 31, 2022

Uh oh!

charris commented Dec 31, 2022

Uh oh!

Uh oh!

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

markopacak commented Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charris commented Jan 17, 2023

Uh oh!

seberg Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

markopacak Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markopacak commented Dec 12, 2022 •

edited

Loading

markopacak Dec 22, 2022 •

edited

Loading

markopacak commented Jan 8, 2023 •

edited

Loading

markopacak Jan 18, 2023 •

edited

Loading