More graceful handling of compressed file Opening

[bcipolli]

This PR deprecates BinOpener, instead exposing a decorator for creating arbitrary file extension / opener methods (similar to that proposed in #317 and created in #319).

The base changes are found in the first commit. The second simply migrates all usage of BinOpener to Opener.

[matthew-brett]

This will modify the Opener class globally no? So all 'Opener' instances will now effectively become BinOpener instances?

[bcipolli]

Exactly. The goal was to abstract Opener such that new opener methods don't require a new class, and that Opener could be used everywhere.

I went this way because Opener/BinOpener seemed to be used by functions expecting it to know about all file types, so making subclasses of Opener when needed, didn't seem like a good option in the end.

But perhaps I'm reading the code poorly... I was looking at bin/nib-nifti-dx, nibabel/arrayproxy.py, and similar.

[matthew-brett]

Forgive my anxiety here, but this decorator, though clever, makes me anxious. It's unexpected to me that a class decorator should alter the global state - in this case of attributes in another class. It makes it harder to reason about Opener. For example, if someone imports Opener from outside nibabel, they will have different file extensions depending on whether freesurfer stuff got imported or no.

Can you see any way round that?

[matthew-brett]

For example, we only need the .mgz compressed extension to be in play for freesurfer files. So maybe we can get away with having a MgzOpener class that is the same as current BinOpener, and putting the class (Opener) into ArrayProxy and FileHolder classes as class attributes. Then the MGH format can subclass these two objects to add the MgzOpener class?

[bcipolli]

if someone imports Opener from outside nibabel, they will have different file extensions depending on whether freesurfer stuff got imported or no.

I believe that this depends what you mean "outside nibabel". If it imports nibabel, then all known image classes are imported, and Opener will have all image extensions. If Opener is imported as a source file / outside of the nibabel package (for example, as a source file to use the base class), then it will not have all extensions. This makes sense to me.

But I feel I'm probably not understanding your concern properly.

For example, we only need the .mgz compressed extension to be in play for freesurfer files. So maybe we can get away with having a MgzOpener class that is the same as current BinOpener, and putting the class (Opener) into ArrayProxy and FileHolder classes as class attributes. Then the MGH format can subclass these two objects to add the MgzOpener class?

I don't like this because each time a new extension is added, a new Opener class needs to be defined, and the code in non-local places needs to change. I find it much cleaner and clearer that everything about an image type should be defined in an image class--how to construct it, read it (Openers), and how to write it. The design you suggest requires adding information outside of that image class.

[matthew-brett]

Reflecting on this a little more - I really don't like the fact that this decorator changes the global state of the Opener class.

We only need this for the .mgz extension - correct? I'm torn then between:

• Leaving as it was before with some better documentation, and possibly renaming BinOpener to ImageOpener or;
• Using your decorator in a class ImageOpener that inherits from Opener, and differs only in having a docstring explaining the horrors that are about to occur.

Ben - do you feel strongly between these?

[bcipolli]

@matthew-brett I still strongly prefer this design.

In my eyes, this is a similar design (at least in my eyes) to the decorator / global state change that was discussed in #317 and used in #319 . Can you help me understand why it's no good here?

The point here is to limit where code needs to change when adding a new opener / extension pairing. If you cahnge the class name every time, then you have to go throughout the code to find places where things may or may not need to change. This seems like a bad design to me.

If you simply add an item to the list of associations in the same global object, then you only add one line of code, in the place where you make the opener / extension association, and that's it. That seems clean to me.

The Opener object would then simply be an interface for storing those pairings.

Not quite understanding the downsides of the suggestion, but definitely see the downsides in the current coding and the suggestion you've made above, if I understand it properly.

[matthew-brett]

If the class is explicitly in order to collect stuff from the other classes, I think this makes sense.

In this case, I intended Opener to be a general file-opening class, that makes sense when reading the code independent of the existence of classes and their variety. It seems like a common case to be able to open a file or a '.gz' file or a '.bz2' file.

Hence my suggestion of ImageOpener, like this:

class ImageOpener(Opener):
     """ Opener-type class passed to image classes to collect compressed extensions
     """

Does that seem like a reasonable compromise?

[bcipolli]

Absolutely. Will work on that soon!

[matthew-brett]

Sorry - should have said, but I'm sure that was obvious - the decorator code then goes in ImageOpener.

[matthew-brett]

I know this is horribly long, but consider register_ext_from_image instead of register_extension ? I find the decorator a bit brain-bending otherwise.

[bcipolli]

@matthew-brett I just successfully rebased my code on the current master, and I think I'll be able to update this PR today.

[bcipolli]

@matthew-brett I've updated to have a class ImageOpener, and added an explicit test of the new decorator.

I believe I use ImageOpener everywhere, I didn't see a particular case where Opener was more appropriate. It's possible I missed one. Other than that, I think things here are in a relatively good state!

Please have a looksie when you have a moment. Thanks!

[matthew-brett]

Maybe add note here on the lines of

This class allows itself to have image extensions added to its class attributes,
via the `register_ex_from_images`.  The class can therefore change state
when image classes are defined.

[bcipolli]

@matthew-brett I updated the docs (including adding a docstring for the decorator itself), and added a deprecation test, which also seems to have appeased coveralls.

[matthew-brett]

Great - thanks for your patience - in it goes.