RF: move load/save logic out of SpatialImage #364

bcipolli · 2015-10-21T13:58:58Z

This PR precedes #360, which builds off of this work. I'll copy some text from that PR; there's quite a bit of discussion about how this fits into forward-looking design of surface vs. spatial, binary vs. xml object models.

Issue:
The final issue blocking streamlined GIFTI/CIFTI in nibabel is: image file I/O functions are tied directly to the SpatialImage class; non-volumetric images (GIFTI/CIFTI) can't extend SpatialImage and so can't easily get in the load/save patheway.

Proposed Solution:
The good news is, the file I/O code actually is quite general. We can easily abstract the file I/O into a higher-level class (which I've called FileBasedHeader/FileBasedImage, as the more generic Header was already taken) that SpatialImage and GiftiImage inherit from.

To do:

Move file I/O from SpatialImage into FileBasedHeader/FileBasedImage
Update tests that assume all image types are of type SpatialImage

Notes:

Given the above design, I believe this is mostly done. Rather than fine-tuning the code now, I'd rather get ideas about the overall design first. @matthew-brett and @effigies , your input is most welcome when you have the time!
Whatever we decide here, I'll be using to push forward GIFTI (WIP: add GIFTI to nib.load/save (and enable non-SpatialImage classes in load/save) #360) and CIFTI (WIP: CIFTI2 spec reader and writer #249) support

effigies · 2015-10-21T16:33:19Z

nibabel/spatialimages.py

-class ImageDataError(Exception):
-    pass
+class Header(SpatialHeader):
+  '''Alias for SpatialHeader; kept for backwards compatibility.'''


Should this be marked deprecated? e.g.

def __init__(self, *args, **kwargs): warnings.warn('Header is deprecated, use SpatialHeader', DeprecationWarning) SpatialHeader.__init__(self, *args, **kwargs)

I should probably note that you'll want stacklevel=2 in that warning.

bcipolli · 2015-10-21T17:08:02Z

Thanks @effigies!

A question: for some comments, it's about old code. Most of the code is copy-pasted from spatialimage.py; it seemed dangerous to move a bunch of code, and make changes at the same time (as git doesn't really track those changes as different).

I'm happy to deal with those things here, but I wanted to double-check first. Thanks!

effigies · 2015-10-21T17:11:19Z

Could be a separate PR, but my thought is that while people are looking at code is a good time to make changes. And the changes will be tracked in the commits, if not the full PR diff.

effigies · 2015-10-21T17:56:33Z

nibabel/filebasedimages.py

+### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ### ##
+''' Common interface for any image format--volume or surface, binary or xml.'''
+
+from ..externals.six import string_types


.externals, not ..externals

Yep. I pushed before running tests; give me a min..

bcipolli · 2015-10-21T18:29:53Z

OK @effigies ; cleaned up and ran tests; pls take a look!

bcipolli · 2015-10-21T18:40:14Z

doctests are failing; will work on those now.

bcipolli · 2015-10-21T20:16:06Z

Made the code updates. A bit worried about deprecating Header now; should we simply document that they're equivalent and it will be deprecated (a la get_header())?

Also a bit concerned about changing the base class of images from Header to SpatialHeader. Would this break pickled headers?

effigies · 2015-10-21T20:19:50Z

@bcipolli Regarding 9cc27ea: For images where there's a filename interface (basically, if there CAN be multiple files, then return the canonical file), then {get,set}_ are preferred. So that commit can be stripped out.

Is there any possibility that XML-based datasets will have multiple files (so a canonical filename is needed)? If not, then {get,set}_filename could be moved back into SpatialImage. But I don't think there's anything wrong with leaving them in FileBasedImage, either.

effigies · 2015-10-21T20:29:25Z

A bit worried about deprecating Header now; should we simply document that they're equivalent and it will be deprecated (a la get_header())?

To be honest, I'm not quite sure what the difference is between posting an intent to deprecate and actually deprecating but not removing. Documentation should be updated ASAP, and DeprecationWarnings are silent by default.

Also a bit concerned about changing the base class of images from Header to SpatialHeader. Would this break pickled headers?

Good question. Is pickling really a thing we care about? The only case I can see is in tests/test_analyze.py. I guess I'm confused why you'd pickle an image rather than save the image. But we can try pickling under Header and loading under SpatialHeader, to be safe.

In any event, I'm also okay with leaving things as subclassed from Header and not SpatialHeader.

bcipolli · 2015-10-21T21:38:14Z

that commit can be stripped out.

Done

Is there any possibility that XML-based datasets will have multiple files

I don't see any reason to keep it off of FileBasedImage; there's nothing to suggest surface images couldn't use a similar approach at some point. CIFTI does something close (using multiple sub-extensions to specify different data types related to a study), but not quite.

Is pickling really a thing we care about?

I'm not sure; perhaps some people or tools use pickle or shelve to store working sessions.

In any event, I'm also okay with leaving things as subclassed from Header and not SpatialHeader.

I'd prefer not to do that; I think that introduces complexity without much of the benefits. I suggest we either use SpatialHeader and deprecate Header, or just have Header without `SpatialHeader.

I'll try this out.

bcipolli · 2015-10-21T23:46:57Z

@effigies I removed the commit and tested shelve (which uses pickle) manually (create a Minc1Header in master, load up in this branch. No issue.

effigies · 2015-10-22T00:32:57Z

Okay. So no barrier to deprecating Header for SpatialHeader?

bcipolli · 2015-10-22T00:33:59Z

Not that I see. I agree that deprecating directly is a fine user experience, from my POV

effigies · 2015-10-22T13:35:29Z

nibabel/filebasedimages.py

+    work:
+    '''
+    files_types = (('image', None),)
+    alternate_exts = ()  # Modified by @ImageOpener.register_ext_from_image


valid_exts

No _meta_sniff_len?

_meta_sniff_len is optional, and on the header.

valid_exts is required, but is also required to be defined by each subclass. Defining it here makes it easier to forget to do that, so I excluded it.

sizeof_hdr is on the header, _meta_sniff_len is on the image and used in path_maybe_image without being checked to exist, first. (It does check whether the header class defines may_contain_header, but that's not tied to presence of _meta_sniff_len, AFAICT.)

files_types must also be defined by subclasses, correct? I think its purpose here is documentary. But the point of my comment was that alternate_exts has been removed from SpatialImage.

effigies · 2015-10-22T13:38:22Z

nibabel/filebasedimages.py

+    @classmethod
+    def is_valid_extension(klass, ext):
+        valid = tuple(ft[1] for ft in klass.files_types) + klass.alternate_exts
+        return ext.lower() in valid


is_valid_extension was removed from SpatialImage in 0c06464.

effigies · 2015-10-22T14:05:37Z

Had another read through, and once we agree on the above issues, I think this can go in.

…I was doing this PR.

bcipolli · 2015-10-22T19:54:06Z

@effigies Made the tweaks requested, tests pass... I think we're good to go! Excited! This is the last refactoring needed to really jump into #249 for CIFTI :)

effigies · 2015-10-23T02:43:05Z

Pushed a PEP8 cleanup for filebasedimages.py, as that was easier than pointing them out. Have a suspicion removing the numpy import will break the doctests. If so, I'll put it into the doctests themselves.

bcipolli · 2015-10-23T02:44:32Z

Pushed a PEP8 cleanup for filebasedimages.py, as that was easier than pointing them out.

Agree, thanks for doing that!

effigies · 2015-10-23T02:48:25Z

Alright. If you're happy with that commit (and the tests pass), in it goes.

bcipolli · 2015-10-23T02:53:09Z

👍

RF: Move load/save logic out of SpatialImage into FileBasedImage Prepares for XML-based images that do not store data as binary arrays. Spatial{Image,Header} now subclass FileBased{Image,Header} Header is a deprecated alias for SpatialHeader

effigies reviewed Oct 21, 2015
View reviewed changes

Ben Cipollini added 2 commits October 21, 2015 13:15

RF: Create abstract FileBasedImage / FileBasedHeader classes.

097d3b1

RF: combine BaseHeader/Image with FileBasedHeader/Image.

f5e5b22

Ben Cipollini added 4 commits October 21, 2015 15:14

RF: Header=>SpatialHeader

249e77a

get_header => header; deprecate get_header

11bd626

Fix filebasedimage docstring.

0226db3

basestring => six.string_types

3ea27d1

Ben Cipollini added 2 commits October 21, 2015 16:33

Remove any mention of affine or dataobj from FileBasedImage

a34220a

make from_image an abstract method

6b40777

effigies reviewed Oct 22, 2015
View reviewed changes

Syncing up with code changes to spatialimages.py that occurred while …

308bbf8

…I was doing this PR.

STY: PEP8 cleanup

975495e

effigies merged commit 9a67e76 into nipy:master Oct 23, 2015

effigies mentioned this pull request Oct 23, 2015

WIP: add GIFTI to nib.load/save (and enable non-SpatialImage classes in load/save) #360

Merged

5 tasks

bcipolli mentioned this pull request Oct 23, 2015

RF: Use XmlSerializable, XmlParser for Gifti load/save #365

Merged

10 tasks

bcipolli deleted the filebasedimage branch February 5, 2016 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RF: move load/save logic out of SpatialImage #364

RF: move load/save logic out of SpatialImage #364

bcipolli commented Oct 21, 2015

effigies Oct 21, 2015

bcipolli Oct 21, 2015

effigies Oct 21, 2015

bcipolli commented Oct 21, 2015

effigies commented Oct 21, 2015

effigies Oct 21, 2015

bcipolli Oct 21, 2015

bcipolli commented Oct 21, 2015

bcipolli commented Oct 21, 2015

bcipolli commented Oct 21, 2015

effigies commented Oct 21, 2015

effigies commented Oct 21, 2015

bcipolli commented Oct 21, 2015

bcipolli commented Oct 21, 2015

effigies commented Oct 22, 2015

bcipolli commented Oct 22, 2015

effigies Oct 22, 2015

bcipolli Oct 22, 2015

effigies Oct 22, 2015

bcipolli Oct 22, 2015

effigies Oct 22, 2015

effigies commented Oct 22, 2015

bcipolli commented Oct 22, 2015

effigies commented Oct 23, 2015

bcipolli commented Oct 23, 2015

effigies commented Oct 23, 2015

bcipolli commented Oct 23, 2015

RF: move load/save logic out of SpatialImage #364

RF: move load/save logic out of SpatialImage #364

Conversation

bcipolli commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcipolli commented Oct 21, 2015

effigies commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcipolli commented Oct 21, 2015

bcipolli commented Oct 21, 2015

bcipolli commented Oct 21, 2015

effigies commented Oct 21, 2015

effigies commented Oct 21, 2015

bcipolli commented Oct 21, 2015

bcipolli commented Oct 21, 2015

effigies commented Oct 22, 2015

bcipolli commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Oct 22, 2015

bcipolli commented Oct 22, 2015

effigies commented Oct 23, 2015

bcipolli commented Oct 23, 2015

effigies commented Oct 23, 2015

bcipolli commented Oct 23, 2015