8000 DOC rollaxis return value is a view of input by jnothman · Pull Request #5258 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

DOC rollaxis return value is a view of input #5258

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
Closed
wants to merge 2 commits into from
Closed

DOC rollaxis return value is a view of input #5258

wants to merge 2 commits into from

Conversation

jnothman
Copy link
Member
@jnothman jnothman commented Nov 4, 2014

No description provided.

seberg
seberg reviewed Nov 4, 2014
View reviewed changes
numpy/core/numeric.py
@@ -1392,7 +1392,7 @@ def rollaxis(a, axis, start=0):
Returns
-------
res : ndarray
Output array.
`a` with its axes permuted. A view is returned whenever possible.
Copy link
Member

Choose a reason for hiding this comment

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

It is always possible to return a view.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I copied from transpose and thought it safer to keep the hedge until it could be confirmed.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, it sounds like transpose documentation should be fixed as well.

@jaimefrio jaimefrio mentioned this pull request Nov 4, 2014
Closed
@charris
Copy link
Member
charris commented Nov 11, 2014

Is this correct for the current verson? I'm thinking of the comments at #5260.

@jaimefrio
Copy link
Member

No it is not:

>>> np.__version__
'1.10.0.dev-0625671'
>>> a = np.arange(60).reshape(3, 4, 5).copy()
>>> a.base is None
True
>>> np.rollaxis(a, 1).base is None
False
>>> np.rollaxis(a, 0).base is None
True

This is easy to fix: in line 1425, replace the current

if axis==start:
    return a

with

if axis==start:
    return a[...]

@charris
8000 Copy link
Member
charris commented Nov 11, 2014

If we make this change, it needs to be mentioned in the 1.10 release notes, maybe in the compatibility section.

@jnothman
Copy link
Member Author

So in no case is the array copied, but currently there are some cases where the input is returned without creating a new view. I don't see why making it always return a view would change any intentional behaviour, but it might be considered a bug-fix.

@charris
Copy link
Member
charris commented Nov 11, 2014

You would be surprised how much code can break because it depends on such "features", often by accident ;)

@jnothman
Copy link
Member Author

Yes, I already know to be surprised... But, really, users taking advantage of a no-op call to mutate the array attributes?

@seberg
Copy link
Member
seberg commented Nov 11, 2014

Frankly, didn't hear complains about arr[...] yet, so probably not very likely, but it is surprising sometimes...

@njsmith
Copy link
Member
njsmith commented Nov 12, 2014

To be fair, it's about as likely in this case that there is code which
already assumes that it always returns a view, and will (continue to) be
broken if we do nothing :-)

On Tue, Nov 11, 2014 at 11:59 PM, seberg notifications@github.com wrote:

Frankly, didn't hear complains about arr[...] yet, so probably not very
likely, but it is surprising sometimes...

—
Reply to this email directly or view it on GitHub
#5258 (comment).

Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org

@charris
Copy link
Member
charris commented Nov 29, 2014

@jnothman Could you make the change suggested by @jaimefrio. Always returning a view seems to be the consensus.

@charris
Copy link
Member
charris commented Jan 19, 2015

Rolled into #5464, so closing this. Thanks @jnothman.

@charris charris closed this Jan 19, 2015
@jnothman
Copy link
Member Author

Ah, yes, sorry I didn't get back on this. I hadn't been sure what the exact fix should be wrt dev comments above. I should have commented so, but I'm glad it's being fixed up in any case.

@jaimefrio jaimefrio mentioned this pull request Mar 19, 2016
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
component: documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jnothman @charris @jaimefrio @seberg @njsmith
0