Skip to content

FIX: Ensure loaded GIFTI files expose writable data arrays #750

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

Merged
merged 3 commits into from
Apr 29, 2019

Conversation

effigies
Copy link
Member

Fixes #746.

@effigies effigies changed the base branch from master to maint/2.4.x April 24, 2019 20:52
@effigies effigies force-pushed the fix/gifti_buffers branch from 2e0f91c to 5edbbe6 Compare April 24, 2019 21:43
@effigies
Copy link
Member Author

So I dug a bit into our options for finding ways of translating from B64BIN/B64GZ to a bytearray with fewer copies, but at least the out-of-the-box attempts like BytesIO + GzipFile will make copies internally and seem unlikely to save us much.

On the plus side, data arrays will rarely get larger than a point set (up to 163842x3 float64s) or triangle mesh (~330000x3 int32s), which will each be less than 4MB, so one or two unnecessary copies are unlikely to cause an out-of-memory error, and the XML parsing will probably be more expensive.

That said, if anybody knows some clever tricks, I'm happy to hear them.

This is ready for review. Travis failures will be fixed with #749.

@effigies effigies added this to the 2.4.1 milestone Apr 25, 2019
@pauldmccarthy
Copy link
Contributor

pauldmccarthy commented Apr 25, 2019

I tried and failed to fix this issue with indexed_gzip*, but I'm wrapping the IndexedGzipFile class with an io.BufferedReader which, regardless of what I do in the IndexedGzipFile class, will always return a bytes object :(

However, I personally don't need the data to be writeable - the only reason I was bitten by this is because pyopengl currently (and unnecessarily) requires writeable arrays. Personally, I'd be inclined to just return read-only data - if the caller needs the data to be writeable, they can explicitly take on the burden of creating the copy.

But as you said, these arrays aren't very big most of the time, so copying is unlikely to be a bottleneck.

*I didn't try very hard, so may have a play with it again some time in the future.

@effigies
Copy link
Member Author

@pauldmccarthy Pretty much all of our image types expose writable data arrays, so I'm inclined to call their being read-only a bug.

@codecov-io
Copy link

codecov-io commented Apr 25, 2019

Codecov Report

Merging #750 into maint/2.4.x will increase coverage by <.01%.
The diff coverage is 91.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##           maint/2.4.x     #750      +/-   ##
===============================================
+ Coverage        88.36%   88.37%   +<.01%     
===============================================
  Files              188      188              
  Lines            23998    23986      -12     
  Branches          4258     4256       -2     
===============================================
- Hits             21207    21197      -10     
+ Misses            2103     2100       -3     
- Partials           688      689       +1
Impacted Files Coverage Δ
nibabel/gifti/parse_gifti_fast.py 84.54% <91.66%> (+0.03%) ⬆️
nibabel/cmdline/parrec2nii.py 32.73% <0%> (ø) ⬆️
...ib/site-packages/nibabel/gifti/parse_gifti_fast.py 84.54% <0%> (+0.03%) ⬆️

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 e7e3668...0c79ad1. Read the comment docs.

@effigies
Copy link
Member Author

Any further comments/reviews?

@effigies effigies merged commit bb4dcd9 into nipy:maint/2.4.x Apr 29, 2019
@effigies effigies deleted the fix/gifti_buffers branch April 29, 2019 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants