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

Support read-only numpy array. #427

Merged
merged 1 commit into from Oct 1, 2014

Conversation

Projects
None yet
3 participants
@MarcCote
Contributor

MarcCote commented Oct 1, 2014

Right now, using Cython version of length and set_number_of_points on read-only numpy arrays (as provided by NiBabel) will fail. The reason is memoryview can only be used with writable numpy array (for more details check https://mail.python.org/pipermail/cython-devel/2013-February/003394.html)

This PR makes sure those Cython functions set the writeable flag of numpy arrays to True before processing them.

Garyfallidis added a commit that referenced this pull request Oct 1, 2014

Merge pull request #427 from MarcCote/fix_streamlinespeed
Support read-only numpy array.

@Garyfallidis Garyfallidis merged commit 56e35fc into nipy:master Oct 1, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 1, 2014

Thx!

@MrBago

This comment has been minimized.

Contributor

MrBago commented Oct 1, 2014

Why does it seem acceptable to change the read status of an argument? At minimum the read status should be changed back once the length computed. There is no guarantee that the read status can safely be changed, read only arrays are often read only for a reason.

I would reject (revert at this point) this PR unless it's meant to address a specific use case.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 1, 2014

We definitely want to use those two functions even though numpy arrays are readonly since we only reads from them by using memoryviews. I agree it is cleaner to change back the writeable flag.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Oct 1, 2014

Sorry I don't follow, could you clarify?

On Wed, Oct 1, 2014 at 11:43 AM, Marc-Alexandre Côté <
notifications@github.com> wrote:

We definitely want to use those two functions even though numpy arrays are
readonly since we only reads from it but using memoryviews. I agree it is
cleaner to change back the writeable flag.


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

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 1, 2014

Sorry, I meant that because memoryviews accept only writeable numpy array, we have to change the flag in order to use these functions: length or set_number_of_points. I made the changes in my branch to changed the writeable flag back to its original state.

How should I proceed to include those changes: make a new PR right now or revert this one first than make a new PR?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 1, 2014

PR is reverted until we figure out if the streamlines were read only for a reason.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 1, 2014

Sorry @MarcCote you will have to redo this PR. Let's go with the safe way of returning the input with the same state as it was given. @matthew-brett do you know why they streamlines are coming out from nibabel as readonly and do you think that there would be a problem with changing their status?

@MrBago

This comment has been minimized.

Contributor

MrBago commented Oct 1, 2014

Why are you getting read only streamlines?
On Oct 1, 2014 11:54 AM, "Marc-Alexandre Côté" notifications@github.com
wrote:

Sorry, I meant that because of memoryviews accept only writeable numpy
array, we have to change the flag in order to use functions length or
set_number_of_points. I made the changes in my branch to changed the
writeable flag back to its original state.

How should I proceed to include those changes: make a new PR right now or
revert this one first than make a new PR?


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

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 1, 2014

In NiBabel when reading streamlines from a trk. More specifically, when using np.ndarray with buffer= argument.

import numpy as np
A = np.array([1, 2, 3])
s = A.tostring()
B = np.ndarray(shape=(3,), dtype="int64", buffer=s)
print A.flags, "\n\n", B.flags

will produces this:

  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  UPDATEIFCOPY : False 

  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : False
  WRITEABLE : False
  ALIGNED : False
  UPDATEIFCOPY : False
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment