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

[bcipolli]

This is for #316. All the recent work has been to get us here :D Thanks @matthew-brett and @effigies !

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.

Caveats:
One danger in doing this is that not all image classes (nibabel.imageclasses.all_image_classes) will be of type SpatialImage. Some tests assume this, and I'm not sure if there's other code out there that might.

To do:

• Move file I/O from SpatialImage into FileBasedHeader/FileBasedImage
• Update tests that assume all image types are of type SpatialImage
• Extend GiftiImage from FileBasedImage
• Deprecate public access to nibabel.gifti.read and nibabel.gifti.write
• Clean up code (a bit messy from merge/unmerge/refactor/rebase, plus my own fiddling)

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 CIFTI support, in WIP: CIFTI2 spec reader and writer #249 .

[bcipolli]

It's worth noting that the code in filebaseimages.py is essentially moved out from spatialimages.py.

[matthew-brett]

In general it would be good to allow images that are not instances of SpatialImage - in the spirit of duck-typing. That's what I was working towards with the image API tests - giving a test rig that could half-assure ourselves that a particular image class had the expected behavior.

[effigies]

Haven't had more than a glance at the code (any chance you could rebase against master so we can look at individual commits?), but the strategy in the solution seems reasonable to me.

That said, maybe another approach is to make SpatialImage a mix-in, rather than a subclass of FileBasedImage. The reason I say this is that a possible long-term strategy would be to describe non-hierarchical interfaces to images, like:

• FileBasedImage defines loading/saving for a file
• SpatialImage defines volumetric access to contents
• SurfaceImage defines surface-vector access to contents
• TractographyImage does something similar for tractography information

Some file formats would support multiple of these interfaces, and having a hierarchy could lead to method-resolution-ordering issues. But maybe this is overcomplicating?

[bcipolli]

@effigies This is interesting, as CIFTI mixes volumetric and surface data. If done well, it could inherit from SpatialImage, SurfaceImage, and FileBasedImage without having to choose between SpatialImage.save and SurfaceImage.save, for example.

The downside, if I'm understanding properly, is that every image will have to have dual inheritance from FileBasedImage and one of the three image classes.

But I'm not too familiar with mixins, honestly.

[effigies]

I think of mixins as superclasses that try not to mess with the class hierarchy.

Sure, they'd have multiple inheritance, but that doesn't strike me as onerous. One compromise would be to have something like SpatialInterface mixin, and then class SpatialImage(SpatialInterface, FileBasedImage) generic that will do the obvious things for any volumetric images that don't need too much format-specific load/save machinery. That way existing SpatialImage inheritors wouldn't need refactoring.

[bcipolli]

I like the cleaner mixin idea; aliasing seems unnecessary to me.
What do you think of the CIFTI example? Is that a "win" for this design?

[effigies]

From your previous comment? Yeah, that's exactly the sort of thing I was thinking.

But I don't really know enough about CIFTI to speak in any greater detail. Is it the case that in some cases you'll want to save it as a volume and others as a surface? Or can it contain both? I know it's XML serializable.

It's worth noting what this idea vould be giving up. Right now SpatialImage seems to be more indicative of the way the data is stored than how it's accessed by the programmer. So if we were to do these data-type-mixins, it might still make sense to have a file-access hierarchy like:

FileBasedImage
    - SpatialImage: data array + header
    - XMLImage: XML serialized image

Sorry, I know I'm jumping all over the place.

[bcipolli]

Not sorry for me--this kind of discussion is why I opened up this PR now. So, thank you!

• CIFTI is currently used as a mix of volume (subcortical) and surface (cortical). I think it's fundamentally declared that way. However, setting things up as inheriting from SpatialImage and SurfaceImage (assuming they're mixins / with non-colliding interfaces) may make it easy to access the volumetric and surface data in a very typical way, rather than defining custom methods. I like that very much.
• I don't think SpatialImage and XmlImage are mutually exclusive; one could store volumetric data in Xml (CIFTI does).

So I would go for:

FileBasedImage
  - XmlBasedImage
SpatialImage
SurfaceImage

Though a problem may be that SpatialImage is not lightweight; I believe it imposes some strong assumptions about storage. Perhaps, to keep mixins clean, then:

FileBasedImage
  - XmlBasedImage
SpatialMixin
SurfaceMixin
class SpatialImage(FileBasedImage, SpatialMixin)
    ...
class CiftiImage(XmlBasedImage, SpatialMixin, SurfaceMixin)

[effigies]

Sorry, I think I was unclear. I meant that I think SpatialImage doesn't refer to the data being volumetric in interpretation but that the actual file is a header and then a binary block of length product(img.shape). So it might make sense to separate SpatialImage the generic file structure from SpatialImage the volumetric way of thinking about the data. (Consider that Nifti and MGH surface files are currently SpatialImages.)

Your final hierarchy looks a bit like what I was suggesting two comments ago. To update with my last proposal:

FileBasedImage
    - SpatialImage
    - XMLImage
VolumeInterface
SurfaceInterface

class GiftiImage(XMLImage, SurfaceInterface)
class CiftiImage(XMLImage, VolumeInterface, SurfaceInterface)
class AnalyzeImage(SpatialImage, VolumeInterface)
class Nifti1Image(SpatialImage, VolumeInterface, SurfaceInterface)

[effigies]

In the interest of simplicity, I cherry-picked your commits. It's very slightly different from this branch, mostly name/ordering/style differences, as well as a couple things that got deleted/restored by your last merge that probably shouldn't have.

Diff. Green is me, red is you.

[bcipolli]

@effigies Thanks for that. Looks good to me.

Your design looks good to me as well. Shall we do that now?

You're welcome to force-push your cherry-picked changes to my branch; you should still have collaborator access.

[effigies]

Pushed.

Sounds good to me. To be clear, at this point I think this simply means abstracting XMLImage out of GiftiImage?

Would you have some way of checking that CiftiImage can be subclassed from XMLImage, so you're not doing work here that needs to be undone when it's time to update cifti?

[effigies]

Some giftiio imports got missed. Hopefully I've cleared them up, but check to see Travis passes before starting work from this branch.

[bcipolli]

Sounds good to me. To be clear, at this point I think this simply means abstracting XMLImage out of GiftiImage?

I think we'd also change how each image class is declared; they would import from SpatialImage and VolumeInterface... if I understood the design above properly.

I'm not clear yet what VolumeInterface (and SurfaceInterface) would look like though, and if it involves removing methods from SpatialImage (so that they're defined on VolumeInterface), I'm honestly a bit nervous about that (for example, other people implementing their own image types will likely be broken).

Any thoughts about that?

Would you have some way of checking that CiftiImage can be subclassed from XMLImage, so you're not doing work here that needs to be undone when it's time to update cifti?

CIFTI is implemented just as GIFTI was before--with to_xml() functions that build xml by strings. I will double-check before starting work, but that's what I think.

[effigies]

So this PR is about getting Gifti (and then Cifti) into the nibabel.{load,save} structure, and not about API for accessing volumetric/surface data, and I'm a bit wary of letting the scope creep. First, it will make this PR take longer, delaying proper Gifti/Cifti support; second, there are two nearly orthogonal parts to the above design, and the XMLImage (or XMLBasedImage) part is the least disruptive and most pertinent to the problem at hand; finally, nobody else has had an opportunity to comment on what has the potential to be a very disruptive design.

I think the sketch-out we did above is useful for having a context for where we think we'll be going (pending broader review), especially for thinking about avoiding creating problems for that future PR, but I think it should be a future PR.

What do you think?

Regarding CIFTI, sounds like that means you can pretty much use the GiftiImage.{to,from}_file_map functions with very little modification. Nice.

[bcipolli]

@effigies Sounds great to me. I was worried about fully fleshing things out now; definitely much broader in scope. I would even suggest leaving the XmlImage refactoring to the CIFTI PR, when there's a need for it, rather than trying to anticipate too much here.

Presumably that'd be done before the next release, and anyway wouldn't interfere with how GIFTI is accessed.

[effigies]

Yeah, holding off on XMLImage sounds fine. So, basically, go with the current design, and try not to do anything that will annoy future devs?

@matthew-brett Pinging you just to give you a sync-point. As far as I'm concerned, we can move on to fiddly details.

[bcipolli]

@effiges in order to simplify things, I think I'll break the refactoring of SpatialImage into it's own PR.

[bcipolli]

@effigies After starting on the cifti branch, I realized it'd be messier to migrate to XmlBasedimage later, so I did it now, on this branch.

Though I realize now we're using two different ways to name. I am using XImage to indicate an image with X content, and XBasedImage to indicate something about the formatting of storage of the image. I think it's helpful to distinguish that in the code.

Lemme know what you think, when you have a chance!

[effigies]

I was mainly using "XMLImage" because it's easier to type. I'd be a little hesitant to use both classes, just because they're similar enough names to cause confusion. But we can beat that horse when we come to it.

Does it make sense to have the to_file_map pulled into the XMLBasedImage? Or do you want to wait for CIFTI to see if it needs any modification?

[bcipolli]

I'm pretty confident that to_file_map will be different for CIFTI. With that said, the gifti one seems pretty generic.

I'll review what's on gifti right now that could be pulled in as meaningful defaults.

[effigies]

If there are definitely going to be common structures decided in #249, I'm okay with leaving XMLBasedImage as a stub in this PR, but if it might just end up as an alias for (FileBasedImage, XMLSerializable), that doesn't seem simplifying enough to justify the last two commits.

This is just my initial thought, but I could be argued out. Don't know how much time I'll have to look at this again before next week, but if you want to rebase against master, that'll clean #364 out of this diff and make it easier to think about.

[bcipolli]

@effiges I've moved over to_file_map. Looking over parse_cifti_fast and parse_gifti_fast, I think it's also possible to generalize the parsing side of things as well.

But as implementing an XmlBasedImage class gets more involved into refactoring, I think it may be best as a separate PR. Thoughts?

[effigies]

Separate PR for refactoring is fine by me. The only thing I wanted to avoid was putting a stub in now that will remain a stub.

[bcipolli]

Sounds good. I have pulled back the commits for XmlBasedImage; will do a separate branch for that, where I think through the common code more thoroughly.

[effigies]

Sounds good. And that diff looks much more manageable. I'll have a detailed look through, soon.

[effigies]

Duplicated valid_exts.

[bcipolli]

Removed duplicate valid_exts, removed warning on import of giftiio.

[effigies]

Did you want to raise deprecation warnings on calls to gifti.read/write? If not, you can drop the warnings import.

I can go either way. It's documented.

[bcipolli]

@effigies I think deprecation warnings to read/write is right; these functions are just shell functions, and nibabel.load/save is the right pathway as far as I'm concerned. I believe these functions were only being used because GiftiImage was not integrated into the canonical load/save pathway.

I've removed the warnings import, as it's not needed--deprecation is done via numpy.deprecate_with_doc.

[effigies]

Looks good. In it goes.