DEP: deprecate calling ma.argsort without an axis by eric-wieser · Pull Request #8918 · numpy/numpy

eric-wieser · 2017-04-10T11:05:16Z

Only deprecated when this would be ambiguous.

Approaches #8701

charris · 2017-04-11T16:34:01Z

numpy/ma/tests/test_deprecations.py

+
+
+class TestArgsort(TestCase):
+	""" gh-8701 """


Should include date and numpy version, that helps keep track of expiration dates, etc.

Thought that having it repeated in the tests would just be noise - date and version are already in the source

eric-wieser · 2017-04-11T16:46:02Z

numpy/ma/core.py

+            # no warning needed - switch to -1 to avoid surprising subclasses
+            return -1
+        else:
+            # 2017-04-10, numpy 1.13.0


Date and version are here - should they be repeated at the call site as well?

And a related idea - maybe we should have a custom np.DeprecationWarning with version and date fields?

I usually grep for the date and version to find what needs to be changed after branching a version, so whatever should be easily grepable and location specific. I think it is not too much work to just add a comment.

charris · 2017-04-11T16:47:05Z

numpy/ma/core.py


        """

+        axis = _deprecate_argsort_axis(self, axis)


Add note with date and numpy version, something like

# 04-11-2017, Numpy 1.13.0, warn on axis default.

Should I remove that comment from inside _deprecate_argsort_axis itself then?

Also below.

Doesn't matter, but really the information needs to be at the point of call.

charris · 2017-04-11T16:51:03Z

numpy/ma/core.py


+def _deprecate_argsort_axis(arr, axis):
+    """
+    Takes the array argsort was called upon, and the axis argument


Should follow usual docstring standard. This is really something for the Parameters section.

charris · 2017-04-11T16:57:48Z

numpy/ma/core.py

+        else:
+            # 2017-04-10, numpy 1.13.0
+            warnings.warn(
+                "Unlike np.argsort, np.sort, np.ma.sort, and the "


Can shorten this message, I'd just say the in the future the default axis will be -1 to match the documentation, not the current None and omit the unlike bit.

Not worth keeping the "pass the axis explicitly to squash" bit?

Can keep that. There is temptation is to explain "why", which is good for a commit message, but documentation should mosty be "what".

Should also mention the change in the function axis documentation with a note about the deprecation.

How do you recommend I word that?

Should we keep the part that says "the default is -1", despite it being wrong?

Maybe

The default value is None. Prior to NumPy 1.13.0 the default was documented to be -1, but that was in error. However, we intend to make the default -1 for real at some future date, and so issue a FutureWarning unless the axis is explicitly given for arrays of dimension > 1.

Note that I think a simple FutureWarning would suffice rather than the temporary two off MaskedArrayFutureWarning.

Are we reserving MaskedArrayFutureWarning for the incoming "mask is the same type of view/non-view as data" change then?

The benefit of having a MaskedArrayFutureWarning is that it makes it obvious that the warning only applies to masked arrays, and the message doesn't need to explain that only ma.argsort has a bad default argument

charris · 2017-04-12T20:08:13Z

Need to finish this up.

eric-wieser · 2017-04-13T01:54:50Z

numpy/ma/core.py

This level needed to be 3, not 2, as it is not directly in the deprecated function

eric-wieser · 2017-04-13T01:56:52Z

numpy/ma/core.py

Doing the test here makes it more obvious that this only affects default arguments

eric-wieser · 2017-04-13T10:16:44Z

numpy/ma/core.py

Added the comment to the axis argument

eric-wieser · 2017-04-13T15:28:31Z

Fun fact: np.ma.maximum and np.ma.minimum also have the wrong default value for the axis argument. but they have no documentation at all. Deprecate those too, or outright fix them?

homu · 2017-04-28T02:38:15Z

☔ The latest upstream changes (presumably #8996) made this pull request unmergeable. Please resolve the merge conflicts.

Only deprecated when this would be ambiguous. Approaches numpy#8701

eric-wieser · 2017-05-05T22:22:41Z

Need to finish this up.

Rebased to fix the merge conflict

charris · 2017-05-06T00:11:19Z

Thanks Eric.

This does not match np.maximum, which is confusing because the masked version has no documentation at all. This uses a similar deprecation approach to numpygh-8918, noting that the warning is only needed for arrays of more than one dimension. The same remarks apply to `np.ma.minimum`

Introduced by me in numpy#8918

This does not match np.maximum, which is confusing because the masked version has no documentation at all. This uses a similar deprecation approach to numpygh-8918, noting that the warning is only needed for arrays of more than one dimension. The same remarks apply to `np.ma.minimum`

Introduced by me in numpy#8918

eric-wieser added 07 - Deprecation component: numpy.ma masked arrays labels Apr 10, 2017

eric-wieser mentioned this pull request Apr 10, 2017

API: Change default axis argument for argsort. #8904

Closed

eric-wieser force-pushed the deprecate-ma-argsort-no-axis branch from 9da0a5b to 1075c1d Compare April 10, 2017 11:24

charris reviewed Apr 11, 2017

View reviewed changes

eric-wieser commented Apr 11, 2017

View reviewed changes

charris reviewed Apr 11, 2017

View reviewed changes

eric-wieser force-pushed the deprecate-ma-argsort-no-axis branch 2 times, most recently from b2b3a0b to 056816c Compare April 13, 2017 01:55

eric-wieser commented Apr 13, 2017

View reviewed changes

numpy/ma/core.py Outdated

Copy link
Copy Markdown

Member Author

eric-wieser Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing the test here makes it more obvious that this only affects default arguments

eric-wieser force-pushed the deprecate-ma-argsort-no-axis branch from 056816c to 844d4f7 Compare April 13, 2017 10:15

eric-wieser commented Apr 13, 2017

View reviewed changes

numpy/ma/core.py Outdated

Copy link
Copy Markdown

Member Author

eric-wieser Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the comment to the axis argument

DEP: deprecate calling ma.argsort without an axis

0a8ee4c

Only deprecated when this would be ambiguous. Approaches numpy#8701

eric-wieser force-pushed the deprecate-ma-argsort-no-axis branch from 844d4f7 to 0a8ee4c Compare May 5, 2017 21:27

charris merged commit 6153a70 into numpy:master May 6, 2017

eric-wieser mentioned this pull request May 6, 2017

DEP: Deprecate np.ma.MaskedArray.mini #8939

Merged

homu mentioned this pull request May 6, 2017

BUG: ndarray.conjugate broken for custom dtypes (unlike np.conjugate) #9003

Closed

eric-wieser added a commit to eric-wieser/numpy that referenced this pull request May 6, 2017

STY: Tabs to spaces

858a4ff

Introduced by me in numpy#8918

mherkazandjian pushed a commit to mherkazandjian/numpy that referenced this pull request May 30, 2017

STY: Tabs to spaces

b5debae

Introduced by me in numpy#8918

Uh oh!

Conversation

eric-wieser commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Apr 11, 2017 • 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!

eric-wieser Apr 11, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

charris Apr 11, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris Apr 11, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris Apr 13, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Apr 13, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

charris commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

homu commented Apr 28, 2017

Uh oh!

eric-wieser commented May 5, 2017

Uh oh!

charris commented May 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

eric-wieser Apr 11, 2017 •

edited

Loading

eric-wieser Apr 11, 2017 •

edited

Loading

charris Apr 11, 2017 •

edited

Loading

charris Apr 11, 2017 •

edited

Loading

charris Apr 13, 2017 •

edited

Loading

eric-wieser Apr 13, 2017 •

edited

Loading

eric-wieser commented Apr 13, 2017 •

edited

Loading