-
Notifications
You must be signed in to change notification settings - Fork 264
MRG: check for MINC using processing routines #464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
For the processing and the visualization routines, we need to know if the spatial axes are first, followed by time etc. Make a crude check that we can depend on spatial axes being first.
ddf6edb
to
4e05ae6
Compare
Current coverage is 94.24% (diff: 98.30%)@@ master #464 diff @@
==========================================
Files 160 161 +1
Lines 21185 21243 +58
Methods 0 0
Messages 0 0
Branches 2265 2276 +11
==========================================
+ Hits 19961 20020 +59
+ Misses 803 802 -1
Partials 421 421
|
So I take it you don't want a Might update the doc on |
Yes, was planning for this to be a short term thing. I take your point about the footguniness - but then I guess the choices are either to add the attribute to all images - I guess with False as the default, to prevent false positives, or list all known image types where spatial axes are first, hoping that people with new image types will know to update that list. What do you think? |
Processing routines should barf on 4D MINC because they do not know how to work out the spatial axes. Use new `spatial_axes_first` function to check for problem images, raise. Test errors raised.
4e05ae6
to
214dd3b
Compare
I added the documentation on required |
Well, if you're okay with adding to the image class, this would be my first thought: class SpatialImage:
...
@property
def spatial_axes_first(self):
return len(self.shape) < 4 And then let known good formats say: class Nifti1Image:
spatial_axes_first = True
... Worth considering whether this should go in the image or the header, but that's the general idea. BUT, if you're thinking of a more long-term axis-labeling, you might not want to have to keep this API around. In which case the existing solution seems fine. |
Yes, right - I was hoping to throw this away fairly soon - and avoid adding to the image API. But of course the definitive fix might not come soon. |
From suggestion by Chris M, make an opt-in list of image types known to be spatial axes first, so that new image types don't accidentally imply they are spatial axes first.
I made an opt-in list of image classes known to be OK. It's a bit ugly but I guess it's a little safer. |
""" | ||
if len(img.shape) < 4: | ||
return True | ||
return type(img) in KNOWN_SPATIAL_FIRST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, this is meant to require exact classes and exclude subclasses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was what I intended, on the basis that it is possible to imagine a sub-class that doesn't have the same behavior, but maybe that is a bit obscure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, that's fine. Just wanted to be sure that was the intended behavior. It makes sense to me to insist on explicit inclusion in the list.
LGTM. |
Thanks for the review. |
The processing routines assume that image have spatial axes before time etc
axes, but this may well not be true for MINC images.
Barf on MINC images > 3D for now. Later maybe we can investigate the image
axis names to work out which axes are spatial.