8000 ENH Generalized rot90 by erensezener · Pull Request #7347 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH Generalized rot90 #7347

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 2 commits into from
Jun 22, 2016
Merged

ENH Generalized rot90 #7347

merged 2 commits into from
Jun 22, 2016

Conversation

erensezener
Copy link
Contributor

This PR generalizes the rot90 function to rotate an arbitrary plane (defined by axes) of a multidimensional array. Our implementation depends on the generalized flip function introduced in PR #7346.

This PR fixes #6506.

This pull request is part of a BCCN Berlin student project by @denisalevi @ge00rg @erensezener

@charris charris added 01 - Enhancement component: numpy.lib 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes labels Feb 26, 2016
@shoyer
Copy link
Member
shoyer commented Feb 26, 2016

This also seems like a good idea, but like for flip, please also run this by the mailing list (could be in the same post).

madphysicist
madphysicist reviewed Feb 26, 2016
View reviewed changes
numpy/lib/twodim_base.py

"""
m = asanyarray(m)

if m.ndim < 2:
raise ValueError("Input must >= 2-d.")
Copy link
Contributor

Choose a reason for hiding this comment

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

You should A) add a check for len(axes) == 2 (and a test that it raises appropriately), B) change this to check that neither axis is >= ndim. Checking for ndim >= 2 is no longer necessary.

Also, if both axes are the same you may want to either raise or return the input array. Either way, you should document how you do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

As @mhvk noted for your previous PR, you should also add support for negative axes.

Copy link
Contributor

Choose a reason for hiding this comment

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

added A) check for len(axes) in 1a52123

added B) removed check for ndim >= 2 in e5b5b96 and added checking for ndim >=2 and a test for it in aa0d4bf

added raise for same axes and a test for it in 83147ee

Copy link
Contributor

Choose a reason for hiding this comment

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

oh and I added negative axes support in ndim check here b277c31

and tests here 2d56768

@charris
Copy link
Member
charris commented Mar 2, 2016

Updated.

@charris
Copy link
Member
charris commented Mar 2, 2016

Needs mention in doc/release/1.12.0-notes.rst. Also squash the 24 commits into one or two. You can use git rebase -i HEAD~24 for that followed by a force push git push --force origin <your branch>.

charris
charris reviewed Mar 2, 2016
View reviewed changes
numpy/lib/twodim_base.py
Reverse the order of elements in an array along the given axis.

The shape of the array is preserved, but the elements are reordered.

Copy link
Member

Choose a reason for hiding this comment

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

Need .. versionadded:: 1.12.0

Copy link
Member

Choose a reason for hiding this comment

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

Why is flip here? Isn't that another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rot90 uses flip so it is based on the other PR. After squashing the commits on the flip PR, we will rebase the rot90 branch on the flip branch.

@madphysicist
Copy link
Contributor

@shoyer, @seberg, @charris. I just wanted to bring up my comment on the flip PR #7347 (comment) again since I don't think there's been a response one way or the other. Is this the right file for these functions given that they do not pertain to 2D arrays? Would function_base be more appropriate?

@shoyer
Copy link
Member
shoyer commented Mar 3, 2016

@madphysicist Yes, I agree -- it would probably make sense to move the generalized versions of these functions out of twodim_base.py.

@charris
Copy link
Member
charris 8000 commented Mar 7, 2016

@erensezener It would be good to move the functions/tests and squash the commits. Same for #7346. Maybe you should do them together as on PR?

@denisalevi denisalevi force-pushed the generalized_rot90 branch 2 times, most recently from a3f56c0 to 595cdfa Compare March 11, 2016 15:37
@denisalevi denisalevi mentioned this pull request Mar 12, 2016
Merged
@denisalevi denisalevi force-pushed the generalized_rot90 branch 2 times, most recently from a8156a9 to ad3a97d Compare March 12, 2016 18:12
@charris
Copy link
Member
charris commented Mar 12, 2016

You should add this to the documentation like was done for flip.
8000

@charris charris removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 13, 2016
@erensezener
Copy link
Contributor Author

@charris Do you mean the release note? It already exists.

@charris
Copy link
Member
charris commented Mar 18, 2016

I was thinking of the official documentation, but I see the function already exists and is there.

Should probably document that the return is a view whenever possible, which AFAICT, is always the case unless wrapping a subtype does something special. I assume flip is the same.

@madphysicist
Copy link
Contributor

Looking at #7421 as an example, it may be a good idea to add a note in the highlights section.

@erensezener
Copy link
Contributor A 8000 uthor

@charris how about:
Returns
-------
y : ndarray
A view of m, which is rotated by 90 degrees in the plane specified by axes.

Also, should we add a note in the highlights section as @madphysicist suggests?

jaimefrio
jaimefrio reviewed Mar 19, 2016
View reviewed changes
numpy/lib/function_base.py
k %= 4

if k == 0:
return m
Copy link
Member

Choose a reason for hiding this comment

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

This will return the array as is if k = 0, which can lead to subtle bugs, see #5258, #5260 and #5468 for some context. Could you please make it always return a view of the input, i.e. return m[:]? It may be worth mentioning it in the documentation, e.g. in the "Returns" section make it "rotated view of the array."

@erensezener
Copy link
Contributor Author

@jaimefrio implemented your suggestions

@erensezener
Copy link
Contributor Author

@charris this is ready for merge as far as I can see.

@charris charris added this to the 1.12.0 release milestone Apr 16, 2016
@charris charris changed the title Generalized rot90 - fixes #6506 ENHL Generalized rot90 May 9, 2016
@charris charris changed the title ENHL Generalized rot90 ENH Generalized rot90 May 9, 2016
@charris charris merged commit de0fcbd into numpy:master Jun 22, 2016
@charris
Copy link
Member
charris commented Jun 22, 2016

Thanks @erensezener .

@MSeifert04 MSeifert04 mentioned this pull request Aug 24, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
01 - Enhancement component: numpy.lib
Projects
None yet
Milestone
1.12.0 release
Development

Successfully merging this pull request may close these issues.

Generalise rot90 to rotate in any plane
7 participants
@erensezener @shoyer @charris @madphysicist @seberg @jaimefrio @denisalevi
0