Push .imageclass.class_map logic into Image classes

bcipolli commented 9 years ago

Adding new images is easier if the information about the image is encapsulated by the Image class. Even better if the SpatialImage super class can define a common interface.

Currently, a number of properties about an image are defined in the imageclasses.py:class_map variable, including (pulling from imageclasses.py):

                'ext': '.img', # characteristic image extension
                'has_affine': False, # class can store an affine
                'makeable': True, # empty image can be easily made in memory
                'rw': True}, # image can be written

I suggest that these all get defined as properties on SpatialImage, and overridden with appropriate values by each subclass. Note that we're already discussing pushing the extension metadata to the image class in #317.

If we think class_map (and ext_map) are being used by external libraries, we can simply build these maps with a simple dictionary comprehension (or list comprehension if Python 2.6 compatibility matters).

effigies commented 9 years ago

+1

class_map and ext_map don't seem particularly necessary once the logic of load/save is pushed into the classes. Since neither of these shows up in the documentation, I lean toward removal rather than reconstruction.

matthew-brett commented 9 years ago

Yes, sure, it would be better to store the information in the classes if we can. I would prefer to reconstruct and deprecate, especially if we want to put this in before the next major release (I mean 3.0).

bcipolli commented 9 years ago

keys to class_map seemed to indicate a 1-to-1 mapping between key and class and class to extension, but that's not the case--MGHImage maps to both mgh and mgz. Thus, the keys to class_map do not indicate image classes, but labels to file extensions.

To keep those keys somewhere, I need to stamp them on the actual class. But it's not a class property--it's a property per extension. Those extensions are defined within the klass.files_types tuple.. which I assume adding a third value to the tuple could be problematic.

It also seems silly to add a new property that duplicates the extensions, just to assign a key/nickname to each.

A bit stuck on the best way to go. almost everything else was easy...

Edited: I also see that while class_map defines every class, the code only uses class_map[ext_map[file_extension]]. The extension map only maps one class to each file extension. Thus, about half the entries to class_map seem to be unused (those that don't appear in class_map because their file_extension overlap with another class).

bcipolli commented 9 years ago

Another thing I don't understand: for the PARREC format, the class defines:

    files_types = (('image', '.rec'), ('header', '.par'))

This seems to indicate that extension .rec is the primary one (it is both the first in the tuple, and associated with the 'image' keyword).

However, class_map and ext_map associate this class with the .par extension. I'm clearly confused about the intended design here.

bcipolli commented 9 years ago

mgz is not defined in files_types in MGHImage, but it is defined in the class_map. I think I should define it in files_types, but ... not sure.

matthew-brett commented 9 years ago

For PARREC - there was no particular logic to choosing 'rec' instead of 'par' as the primary one, other than the parallel with nifti pairs, where 'img' is more generally used to refer to the the image, than 'hdr'. It was accidental that ext_map and class_map use par instead.

bcipolli commented 9 years ago

It was accidental that ext_map and class_map use par instead.

Accidental, but I think it matters. When I switch them to use rec instead of par, the tests break.

Indeed, I would have expected that ext_map and class_map allow loading regardless of whether you specify the header or image file, and that those are tested. I do not believe that's currently the case.

For example, when I change the parrec tests to use EG_REC, I get:

  File "/Users/bcipolli/code/_git_libs/nibabel/nibabel/tests/test_spatialimages.py", line 371, in test_load_mmap
    back_img = func(param1, **kwargs)
  File "/Users/bcipolli/code/_git_libs/nibabel/nibabel/loadsave.py", line 44, in load
    return guessed_image_type(filename).from_filename(filename, **kwargs)
  File "/Users/bcipolli/code/_git_libs/nibabel/nibabel/loadsave.py", line 66, in guessed_image_type
    filename)
ImageFileError: Cannot work out file type of "/Users/bcipolli/code/_git_libs/nibabel/nibabel/tests/data/phantom_EPI_asc_CLEAR_2_1.REC"

So I can't just start using rec--anybody using PARREC must be passing in the .par file. And I'm not sure what unexpected effects switching the order of par and rec in files_types would have.

My only idea is to try and add both the headers and the images to the class_map and ext_map. I can do that, but that'll take a bit of work to expand the test coverage.

matthew-brett commented 9 years ago

For the design of class_map and ext_map: class_map and ext_map are to give the best guess at a SpatialImage class for a given extension.

For save, we:

Check whether the input image class is compatible with the required extension, and if so, use the input image class; OTHERWISE:
Use the image type generated from ext_map and then the image class for the given image type (from class_map to find the class to use.

For load we:

Guess the class to load with using img_type = ext_map[ext]; img_class = class_map[img_type], along with the other heuristics for when the image extension can indicate more than one class.

files_types has a two purposes:

To define the 'characteristic' extension for the file (implicitly defined by the first entry in files_types);
To define the file_map of the image, where each entry in files_types defines a file meaning ('image' or 'header', usually) and a file extension. So the Nifti pair files_types is: (('image','.img'), ('header','.hdr')), meaning that, the image consists of two files - the so-called 'image' file, with .img extension, and the so-called 'header' file with the .hdr extension.

effigies commented 9 years ago

Not at my machine so can't test, but I believe my PR (#319) is able to load from either PAR or REC. Using that style of IMAGE_MAP was the trick, I think.

On July 9, 2015 12:08:32 PM EDT, Ben Cipollini notifications@github.com wrote:

It was accidental that ext_map and class_map use par instead.

Accidental, but I think it matters. When I switch them to use rec instead of par, the tests break.

Indeed, I would have expected that ext_map and class_map allow loading regardless of whether you specify the header or image file, and that those are tested. I do not believe that's currently the case.

For example, when I change the parrec tests to use EG_REC, I get:
File
"/Users/bcipolli/code/_git_libs/nibabel/nibabel/tests/test_spatialimages.py",
line 371, in test_load_mmap
   back_img = func(param1, **kwargs)
File "/Users/bcipolli/code/_git_libs/nibabel/nibabel/loadsave.py", line
44, in load
 return guessed_image_type(filename).from_filename(filename, **kwargs)
File "/Users/bcipolli/code/_git_libs/nibabel/nibabel/loadsave.py", line
66, in guessed_image_type
   filename)
ImageFileError: Cannot work out file type of
"/Users/bcipolli/code/_git_libs/nibabel/nibabel/tests/data/phantom_EPI_asc_CLEAR_2_1.REC"
So I can't just start using rec--anybody using PARREC must be passing in the .par file. And I'm not sure what unexpected effects switching the order of par and rec in files_types would have.

My only idea is to try and add both the headers and the images to the class_map and ext_map. I can do that, but that'll take a bit of work to expand the test coverage.

Reply to this email directly or view it on GitHub: https://github.com/nipy/nibabel/issues/323#issuecomment-120050912

Chris Markiewicz

bcipolli commented 9 years ago

@effigies Oh, I didn't see that one, my bad... I guess I"m not subscribed to nibabel updates :-/ Will take a look.

matthew-brett commented 9 years ago

Yes, it does matter which of rec or par is first, and it would be better to allow both.

Why not give up on class_map and ext_map for now, they are just hacks that we can remove when we have a better solution. Maybe we'll end up leaving the current code for class_map and ext_map in place for backwards compatibility, while removing their use and deprecating.

bcipolli commented 9 years ago

@matthew-brett Sure thing. I was just trying to re-create them programmatically for backwards-compatibility, but I can just leave them as-is and simply stop using them in the code.

effigies commented 9 years ago

Closed by #329.

nipy / nibabel

Push .imageclass.class_map logic into Image classes #323