ENH: Add SerializableImage class with to/from_bytes methods #644

effigies · 2018-07-02T19:21:39Z

Closes #639.

Implements to/from_bytes for single-image variants of NIfTI-1, NIfTI-2, GIFTI and CIFTI2 images, as well as MGH images.

Created an abstract class deriving from FileBasedImage that can be used in other images, if it becomes desirable, but didn't want to spend a ton of time nailing it down for image types I have an, at best, passing familiarity with.

codecov-io · 2018-07-02T20:21:46Z

Codecov Report

Merging #644 into master will decrease coverage by 1.56%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #644      +/-   ##
=========================================
- Coverage   89.97%   88.4%   -1.57%     
=========================================
  Files          94     188      +94     
  Lines       12004   24038   +12034     
  Branches     2127    4254    +2127     
=========================================
+ Hits        10800   21251   +10451     
- Misses        862    2100    +1238     
- Partials      342     687     +345

Impacted Files	Coverage Δ
nibabel/freesurfer/mghformat.py	`95.45% <100%> (+0.01%)`	⬆️
nibabel/__init__.py	`93.75% <100%> (+0.13%)`	⬆️
nibabel/filebasedimages.py	`90.71% <100%> (+1.56%)`	⬆️
nibabel/nifti1.py	`91.18% <100%> (-0.3%)`	⬇️
nibabel/gifti/gifti.py	`95.23% <100%> (+0.01%)`	⬆️
nibabel/environment.py	`75% <0%> (-20%)`	⬇️
nibabel/casting.py	`85.59% <0%> (-1.7%)`	⬇️
nibabel/dft.py	`80.48% <0%> (-0.61%)`	⬇️
nibabel/cmdline/parrec2nii.py	`32.73% <0%> (ø)`	⬆️
... and 95 more

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 eb3bd12...9eccf32. Read the comment docs.

coveralls · 2018-07-02T20:22:50Z

Coverage increased (+0.02%) to 91.846% when pulling 2407015 on effigies:enh/serialize into be35aca on nipy:master.

effigies · 2018-07-31T16:21:02Z

Current state: CIFTI issues. Not clear why it's not consistent with the file output, but may be due to to_file_map performing updates. Perhaps they're not idempotent.

Based on TimVanMourik/Armadillo#11, it seems that deserializing would be similarly useful.

Incidentally, I'm not committed to "(de)serialize" as function names. To be consistent with the rest of the API, we could use to_bytes and from_bytes. Let me know what y'all think.

Shotgunosine · 2018-07-31T16:29:37Z

to_bytes and from_bytes sounds good to me. Easier for a novice to interpret as well.

…

On Tue, Jul 31, 2018 at 12:21 PM, Chris Markiewicz ***@***.*** > wrote: Current state: CIFTI issues. Not clear why it's not consistent with the file output, but may be due to to_file_map performing updates. Perhaps they're not idempotent. Based on TimVanMourik/Armadillo#11 <TimVanMourik/Armadillo#11>, it seems that deserializing would be similarly useful. Incidentally, I'm not committed to "(de)serialize" as function names. To be consistent with the rest of the API, we could use to_bytes and from_bytes. Let me know what y'all think. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFC1mG82We8kHI6gSc4gTJf9nM9IyOdAks5uMIPvgaJpZM4U_0Of> .

effigies · 2018-08-14T02:55:14Z

I think this is ready for a review, if anybody has some time. Pretty compact, in the end. I may not have used the testing machinery to its full potential, but hopefully it isn't too grotesque.

matthew-brett

I'm a bit worried by the heuristic of simply dumping the header and the image into the same file, if the format does not specify how that should be done.

Maybe we could allow file formats to specify how that should be done, but barf if they do not.

We could start by barfing with any file format that has more than one image, and then build up?

matthew-brett · 2018-08-16T10:37:05Z

nibabel/filebasedimages.py

+
+       * from_bytes(bytestring) - make instance by deserializing a byte string
+
+    The following properties should hold:


How do you want this to look in the rendered doc? What happens with these indented lists?

I actually haven't built the docs here. I'll fiddle with this.

matthew-brett · 2018-08-16T10:38:27Z

nibabel/filebasedimages.py

+
+    The following properties should hold:
+
+      * ``klass.from_bytes(bstr).to_bytes() == bstr``


Is that true in general for round-tripping? For example, what happens with nifti images with image scaling? I think the scale factors may get recalculated.

Fair point. I guess what we want is load-level idempotence, to the same level of precision as save/load cycles.:

img_a = klass.from_bytes(bstr) img_b = klass.from_bytes(img_a.to_bytes()) np.allclose(img_a.get_fdata(), img_b.get_fdata())

Would the following be too strong, as well?

img_a.to_bytes() == img_b.to_bytes()

nibabel/filebasedimages.py

nibabel/gifti/gifti.py

nibabel/tests/test_image_api.py

effigies · 2018-09-26T16:09:38Z

@matthew-brett I've addressed (or responded to) your comments, if you want to have another look.

effigies · 2019-06-20T12:35:35Z

I've updated this PR to do the following:

The mixin methods to_bytes/from_bytes now check whether the class's files_types attribute indicates a multi-file image, and raises a NotImplementedError. This can be overridden by a multi-image class, but ensures we don't accidentally make promises we can't keep.
Nifti1Pair was marked serializable, when it should have been Nifti1Image. This has been corrected.
The header equality check in the API tests is now more specifically targeted to the one corner case, i.e., the abstract FileBasedHeader class.

@matthew-brett I believe this has addressed all of your comments. Please let me know if you have any further concerns.

I would like to get this in before the final Python 2 feature release (2.5.0) if possible.

If anybody else has some spare cycles, I'd appreciate a review on this one.

matthew-brett

Thanks - yes - you've covered the stuff I was worried about.

matthew-brett · 2019-06-20T13:47:09Z

nibabel/filebasedimages.py

+            fobj.write(img.to_bytes())
+
+    Images that consist of separate header and data files (e.g., Analyze
+    images) currently do not support this interface.


Maybe say something about the fact that image types can override this class method to implement it, for multi-file images.

matthew-brett · 2019-06-20T13:47:31Z

nibabel/filebasedimages.py

+    Images that consist of separate header and data files (e.g., Analyze
+    images) currently do not support this interface.
+    '''
+    @classmethod


PEP8 doesn't need a newline here?

matthew-brett · 2019-06-20T13:50:06Z

nibabel/tests/test_volumeutils.py

@@ -700,7 +700,7 @@ def test_a2f_non_numeric():
    # Some versions of numpy can cast structured types to float, others not
    try:
        arr.astype(float)
-    except ValueError:
+    except (TypeError, ValueError):


Why do you need this change in this PR?

Ah, nope. Rebased onto the wrong branch.

…ageAPI

effigies · 2019-06-22T11:20:43Z

Thanks for the review, Matthew.

effigies mentioned this pull request Jul 17, 2018

FileBasedImage.serialize() #639

Closed

effigies mentioned this pull request Jul 31, 2018

[BUG] Remove hard-coded GifTI format 'GZipBase64Binary' parsing TimVanMourik/Armadillo#11

Closed

effigies changed the title ~~[WIP] ENH: Add FileBasedImage.serialize()~~ [WIP] ENH: Add SerializableImage class with to/from_bytes methods Aug 11, 2018

effigies changed the title ~~[WIP] ENH: Add SerializableImage class with to/from_bytes methods~~ ENH: Add SerializableImage class with to/from_bytes methods Aug 14, 2018

matthew-brett reviewed Aug 16, 2018

View reviewed changes

This was referenced Aug 30, 2018

Ingest datasets without storing actual niftis neuroscout/neuroscout#342

Closed

RF: Use GiftiImage.to_xml over BytesIO NeuroVault/NeuroVault#654

Merged

effigies force-pushed the enh/serialize branch from 67dc4fa to 2407015 Compare September 26, 2018 16:09

effigies mentioned this pull request Oct 1, 2018

REL: 2.3.1 #667

Merged

19 tasks

effigies added this to the 2.4.0 milestone Feb 12, 2019

effigies mentioned this pull request Mar 15, 2019

REL: 2.4.0 #740

Merged

20 tasks

effigies modified the milestones: 2.4.0, 2.5.0 Mar 18, 2019

effigies force-pushed the enh/serialize branch from 2407015 to f23aed6 Compare May 25, 2019 21:42

effigies force-pushed the enh/serialize branch from f23aed6 to 9eccf32 Compare June 20, 2019 12:22

matthew-brett reviewed Jun 20, 2019

View reviewed changes

effigies added 7 commits June 20, 2019 10:44

ENH: Add FileBasedImage.serialize()

bb64f19

DOC: FileBasedImage.serialize.__doc__

75ed05a

FIX: Import io

c278a94

ENH: Test serialize via an API test mixin

ca60a19

RF: Rename serialize to to_bytes, add from_bytes

38fd9fc

RF: Factor SerializableImage

b729779

TEST: Generate file when no examples available

c943e9d

effigies added 8 commits June 20, 2019 10:44

TEST: klass sometimes missing, equality sometimes undefined

96bc5a0

ENH: Add to/from_bytes interface to GiftiImage

08a7bab

TEST: Delete images so mmapped files can be removed

372ac68

DOC: Improve docstring

e197b72

FIX: Nifti1Image is serializable, not Nifti1Pair

eae0342

ENH: Check for multi-file images, to ensure well-defined behavior

30bea56

TEST: Move special case header equality check into TestSerializableIm…

562fac8

…ageAPI

STY/DOC: Note about multi-file images, newline

f829919

effigies force-pushed the enh/serialize branch from 9eccf32 to f829919 Compare June 20, 2019 14:44

effigies merged commit 980f678 into nipy:master Jun 22, 2019

effigies deleted the enh/serialize branch June 22, 2019 11:20


		* from_bytes(bytestring) - make instance by deserializing a byte string

		The following properties should hold:


		The following properties should hold:

		* ``klass.from_bytes(bstr).to_bytes() == bstr``

ENH: Add SerializableImage class with to/from_bytes methods #644

ENH: Add SerializableImage class with to/from_bytes methods #644

Uh oh!

Conversation

effigies commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coveralls commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Jul 31, 2018

Uh oh!

Shotgunosine commented Jul 31, 2018 via email

Uh oh!

effigies commented Aug 14, 2018

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

effigies commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Jun 20, 2019

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Jun 22, 2019

Uh oh!

Uh oh!

effigies commented Jul 2, 2018 •

edited

Loading

codecov-io commented Jul 2, 2018 •

edited

Loading

coveralls commented Jul 2, 2018 •

edited

Loading

effigies commented Sep 26, 2018 •

edited

Loading