8000 DEP: deprecate rollaxis by nschloe · Pull Request #9475 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

DEP: deprecate rollaxis #9475

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 11, 2017
Merged

DEP: deprecate rollaxis #9475

merged 7 commits into from
Aug 11, 2017

Conversation

nschloe
Copy link
Contributor
@nschloe nschloe commented Jul 27, 2017

The changelog of 1.11 says about the then-new moveaxis:

This function should be easier to use than the current rollaxis function as well as providing more functionality.

I've recently stumbled upon rollaxis's weirdness myself, so perhaps it's time to deprecate it.

This PR replaces all occurrences of rollaxis by its corresponding moveaxis equivalent, and arguably the code becomes clearer. For example, instead of

c = np.rollaxis(c, iaxis)
# do something
c = np.rollaxis(c, 0, iaxis + 1)

the new

c = np.moveaxis(c, iaxis, 0)
# do something
c = np.moveaxis(c, 0, iaxis)

makes clear that iaxis moved to the first spot, then something happens, and then it's moved back.

eric-wieser
eric-wieser reviewed Jul 27, 2017
View reviewed changes
numpy/linalg/linalg.py Outdated
@@ -2006,7 +2006,7 @@ def _multi_svd_norm(x, row_axis, col_axis, op):
"""
if row_axis > col_axis:
row_axis -= 1
y = rollaxis(rollaxis(x, col_axis, x.ndim), row_axis, -1)
y = moveaxis(moveaxis(x, col_axis, -1), row_axis, -2)
Copy link
Member

Choose a reason for hiding this comment

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

This can be done with a single call to moveaxis

@shoyer
Copy link
Member
shoyer commented Jul 27, 2017 •
edited
Loading

I'm +1 for moving internal uses of rollaxis to moveaxis.

I'm -1 on actually deprecating rollaxis. A louder warning in the docstring for rollaxis against using it would be great (e.g., "We recommend using moveaxis instead."). Take a look at the docs for hstack, for example: https://docs.scipy.org/doc/numpy/reference/generated/numpy.hstack.html

We don't actually remove functionality in NumPy unless absolutely necessary, and we don't deprecate anything unless we actually plan to remove it. rollaxis has a confusing API but keeping it with a warning is pretty harmless.

@eric-wieser
Copy link
Member

Don't forget the DEP: and MAINT: commit prefices. Also, this fails because you're not hiding deprecation warnings in rollaxis tests

@eric-wieser
Copy link
Member

@shoyer: to be clear, by warning you mean documentation-only, as opposed to an actual python warning.

@eric-wieser
Copy link
Member

Perhaps we should introduce a OutdatedWarning that's indicates a feature is there to stay, but has a better replacement?

@charris
Copy link
Member
charris commented Jul 27, 2017

I'm with @shoyer on this one.

@charris charris changed the title deprecate rollaxis DEP: deprecate rollaxis Jul 27, 2017
@shoyer
Copy link
Member
shoyer commented Jul 27, 2017

Perhaps we should introduce a OutdatedWarning that's indicates a feature is there to stay, but has a better replacement?

Sure, but only if we can silence the warning by default. Otherwise it's an even more annoying version of DeprecationWarning -- one that's entirely useless!

A red box like what you find in the Python docs for deprecated functions could be a nice approach:
https://docs.python.org/2/library/string.html#deprecated-string-functions

(Notice that none of these functions actually issue a warning when you use them.)

@nschloe
Copy link
Contributor Author
nschloe commented Jul 27, 2017

Thanks for the comments, I've addressed the issues mentioned so far.

As for the deprecation, I would say there must be some sort of it. This little text found in hstack docstring (and rollaxis after merging this PR) is hardly enough for me to notice it, so I'd vote for something more striking. If you don't want to annoy users with console messages, perhaps @shoyer's suggestiong of red boxes is the way to do. Seems pythonic anyways.

Should we tackle this now or on another PR?

@shoyer
Copy link
Member
shoyer commented Jul 27, 2017

Should we tackle this now or on another PR?

Maybe another PR? There's a long list of semi-deprecated functions. Clarifying that could require some discussion.

@nschloe nschloe mentioned this pull request Jul 27, 2017
Closed
@nschloe
Copy link
Contributor Author
nschloe commented Jul 27, 2017

I've added #9478 so we don't lose track of this.

eric-wieser
eric-wieser reviewed Jul 27, 2017
View reviewed changes
numpy/core/numeric.py Outdated
@@ -1436,6 +1436,10 @@ def rollaxis(a, axis, start=0):
"""
Roll the specified axis backwards, until it lies in a given position.

This function continues to be supported for backward compatibility, but you
should prefer np.moveaxis. The np.moveaxis function was added in NumPy
1.11.
Copy link
Member

Choose a reason for hiding this comment

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

referring to this as `moveaxis` will cause a link to be generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it because of the missing tick or the additional np.?

Copy link
Member

Choose a reason for hiding this comment

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

Both, I think. There's nothing special about "np" - remember, the module's called "numpy" (but that's implied anyway in references)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

eric-wieser
eric-wieser 8000 reviewed Jul 27, 2017
View reviewed changes
numpy/linalg/linalg.py Outdated
@@ -2006,7 +2006,7 @@ def _multi_svd_norm(x, row_axis, col_axis, op):
"""
if row_axis > col_axis:
row_axis -= 1
y = rollaxis(rollaxis(x, col_axis, x.ndim), row_axis, -1)
y = moveaxis(x, [row_axis, col_axis], [-2, -1])
Copy link
Member

Choose a reason for hiding this comment

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

Might be inclined to pass tuples here, if only because the function used internally is called normalize_index_tuple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the tests are failing here. travis says

ValueError: repeated axis in source argument

so I'm guessing this occurs when row_axis and col_axis are equal. Perhaps we do need two moveaxis here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit lost as to why if row_axis > col_axis: row_axis -= 1 is done.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a crutch for the rollaxis behaviour to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit that introduced this function is 4 years old, a292614, and the code hasn't been touched since.

Copy link
Member

Choose a reason for hiding this comment

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

And in fact, after removing it, there's an assertion for nonequality at the caller

Copy link
Member

Choose a reason for hiding this comment

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

So to conclude: just remove those two lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I get it now: If if row_axis > col_axis, and col_axis is moved to the end of the shape first, one needs to row_axis -= 1 to still get the right axis when moving the second one to the end.

Indeed, we don't need this anymore. Praised be moveaxis!

eric-wieser
eric-wieser approved these changes Jul 27, 2017
View reviewed changes
Copy link
Member
@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

LGTM, but needs squashing - we can do that from GitHub's ui if needed

@charris
Copy link
Member
charris commented Aug 7, 2017

@nschloe Could you add a comment to doc/release/1.14.0-notes.rst?

@nschloe
Copy link
Contributor Author
nschloe commented Aug 7, 2017

@charris Under deprecations?

@charris
Copy link
Member
charris commented Aug 7, 2017

Oh, sorry. I see that no actual warning is issued, so no note is needed.

@mhvk
Copy link
Contributor
mhvk commented Aug 8, 2017

Late here, but hurray for this cleanup! (and for changing the main documentation to make it clearer that rollaxis is no longer something we recommend people to use)

@charris
Copy link
Member
charris commented Aug 10, 2017

@eric-wieser Want to finish this up?

@eric-wieser eric-wieser merged commit 029863e into numpy:master Aug 11, 2017
@eric-wieser
Copy link
Member

@charris: Thought I'd give time for @nschloe to do the squash themself

@eric-wieser
Copy link
Member
eric-wieser commented Aug 11, 2017 •
edited
Loading

Thanksfor the patch @nschloe!

@eric-wieser eric-wieser mentioned this pull request Aug 11, 2017
Closed
@eric-wieser
Copy link
Member

Interestingly this seems to cause a significant performance decrease. I don't think we should reverse this, but maybe move_axis should be made faster somehow.

@eric-wieser eric-wieser mentioned this pull request Dec 12, 2017
Closed
@eric-wieser eric-wieser mentioned this pull request Aug 1, 2018
Open
@eric-wieser eric-wieser mentioned this pull request Apr 8, 2020
Closed
@rossbar rossbar mentioned this pull request Apr 20, 2020
Merged
@AnirudhDagar AnirudhDagar mentioned this pull request Jul 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-wieser eric-wieser eric-wieser approved these changes

Assignees
No one assigned
Labels
03 - Maintenance 07 - Deprecation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nschloe @shoyer @eric-wieser @charris @mhvk
0