-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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.
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
ENH Generalized rot90 #7347
ENH Generalized rot90 #7347
Conversation
This also seems like a good idea, but like for |
|
||
""" | ||
m = asanyarray(m) | ||
|
||
if m.ndim < 2: | ||
raise ValueError("Input must >= 2-d.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. |
Needs mention in |
Reverse the order of elements in an array along the given axis. | ||
|
||
The shape of the array is preserved, but the elements are reordered. | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@shoyer, @seberg, @charris. I just wanted to bring up my comment on the |
@madphysicist Yes, I agree -- it would probably make sense to move the generalized versions of these functions out of twodim_base.py. |
@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? |
a3f56c0
to
595cdfa
Compare
595cdfa
to
02d3cda
Compare
c929e8a
to
7faa4d0
Compare
a8156a9
to
ad3a97d
Compare
ad3a97d
to
7a57783
Compare
You should add this to the documentation like was done for flip. |
@charris Do you mean the release note? It already exists. |
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. |
Looking at #7421 as an example, it may be a good idea to add a note in the highlights section. |
@charris how about: Also, should we add a note in the highlights section as @madphysicist suggests? |
k %= 4 | ||
|
||
if k == 0: | ||
return m |
There was a problem hiding this comment.
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."
7a57783
to
1dd7108
Compare
@jaimefrio implemented your suggestions |
@charris this is ready for merge as far as I can see. |
Thanks @erensezener . |
This PR generalizes the
rot90
function to rotate an arbitrary plane (defined byaxes
) of a multidimensional array. Our implementation depends on the generalizedflip
function introduced in PR #7346.This PR fixes #6506.
This pull request is part of a BCCN Berlin student project by @denisalevi @ge00rg @erensezener