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

Cast Streamline attrs to numpy ints, to avoid buffer mismatch. #1263

Merged
merged 2 commits into from Jun 19, 2017

Conversation

Projects
None yet
4 participants
@@ -102,15 +102,18 @@ def length(streamlines):
arclengths = np.zeros(len(streamlines), dtype=np.float64)
if streamlines.data.dtype == np.float32:
c_arclengths_from_arraysequence[float2d](streamlines.data,
streamlines._offsets,

This comment has been minimized.

@matthew-brett

matthew-brett Jun 18, 2017

Member

What type are the offsets, lengths normally? I guess they should be np.intp type. I guess this conversion could have a cost, most efficient might be guarantee their type on streamline object creation, then you can do streamlines._offsets.astype(np.np_intp) here assuming it will have a very small cost (just the type check).

This comment has been minimized.

@arokem

arokem Jun 18, 2017

Member

Streamlines is a nibabel.streamlines.ArraySequence object! So, I guess this is a question to the nibabel developers :-)

More seriously, from: https://github.com/nipy/nibabel/blob/master/nibabel/streamlines/array_sequence.py#L77-L78 it does look like that's a np.intp! Changed that cast to intp instead.

This comment has been minimized.

@matthew-brett

matthew-brett Jun 19, 2017

Member

@MarcCote - any thoughts here? I guess these streamlines are somehow of type long, but they should be of type np.intp. Any way of making them be np.intp from the start, to avoid memory costs of the conversion?

@codecov-io

This comment has been minimized.

codecov-io commented Jun 18, 2017

Codecov Report

Merging #1263 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1263   +/-   ##
=======================================
  Coverage   87.05%   87.05%           
=======================================
  Files         226      226           
  Lines       28262    28262           
  Branches     3026     3026           
=======================================
  Hits        24603    24603           
  Misses       2976     2976           
  Partials      683      683

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efb7170...7a2f4da. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Jun 18, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling c21b535 on arokem:sl-length-as-npint into efb7170 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 18, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling c21b535 on arokem:sl-length-as-npint into efb7170 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 18, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 7a2f4da on arokem:sl-length-as-npint into efb7170 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 18, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 7a2f4da on arokem:sl-length-as-npint into efb7170 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Jun 19, 2017

Seems to have resolved the original problem: https://nipy.bic.berkeley.edu/builders/dipy-bdist64-35/builds/123 (from a try_branch run with this branch).

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jun 19, 2017

OK - looks good to me - @MarcCote - I'm still interested whether there's a way of making sure the type is np.intp on creation.

@matthew-brett matthew-brett merged commit b7c093d into nipy:master Jun 19, 2017

4 checks passed

codecov/patch Coverage not affected when comparing efb7170...7a2f4da
Details
codecov/project 87.05% remains the same compared to efb7170
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 85.656%
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1263 from arokem/sl-length-as-npint
MRG: Cast Streamline attrs to numpy ints, to avoid buffer mismatch.

Fixes casting error seen at See: https://nipy.bic.berkeley.edu/builders/dipy-bdist64-27/builds/122/steps/shell_12/logs/stdio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment