ENH: Add get/set_zooms(units='norm') to manipulate zooms in mm/s units

[effigies]

Because most NIfTI files are in mm+s units, it's pretty easy to write code that breaks on nonstandard units. This PR adds a get_norm_zooms method to spatial image headers that always returns zooms scaled to mm+s, based on metadata when available.

The basic API is:

img.header.get_norm_zooms()  # Zooms in mm+s, assuming already in mm+s if missing
img.header.get_norm_zooms(raise_unknown=True)  # Raise error if units not explicitly encoded

Edit:

With MGHHeader encoding TR in milliseconds (see snippet), being able to access zooms in standard units across image types seems even more like a good idea.

https://github.com/nipy/nibabel/blob/b8545ef665e5a045d67ff9c2af9e677bddbe0962/nibabel/freesurfer/mghformat.py#L240-L246

I've also added a set_norm_zooms() that takes zooms in mm/s and applies any necessary transformations to set the headers correctly to ensure a lossless round-trip with get_norm_zooms.

[coveralls]

Coverage decreased (-0.1%) to 96.149% when pulling 2bfc8d7e03c4d4803a9df3bd83e8b72853aaef66 on effigies:enh/norm_units into b2c0f81584efff9c3244bce7874656fd5a7e351b on nipy:master.

[codecov-io]

Codecov Report

Merging #567 into master will decrease coverage by 0.19%. The diff coverage is 66.29%.

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
- Coverage   91.73%   91.54%   -0.20%     
==========================================
  Files          97       97              
  Lines       12359    12433      +74     
  Branches     2177     2205      +28     
==========================================
+ Hits        11338    11382      +44     
- Misses        684      700      +16     
- Partials      337      351      +14     

Impacted Files	Coverage Δ
nibabel/spatialimages.py	94.52% <37.50%> (-1.92%)	:arrow_down:
nibabel/analyze.py	97.36% <42.85%> (-1.16%)	:arrow_down:
nibabel/freesurfer/mghformat.py	94.20% <66.66%> (-1.32%)	:arrow_down:
nibabel/nifti1.py	89.91% <69.64%> (-1.76%)	:arrow_down:
nibabel/ecat.py	88.14% <100.00%> (+0.06%)	:arrow_up:
nibabel/minc1.py	90.75% <100.00%> (+0.10%)	:arrow_up:
nibabel/volumeutils.py	83.94% <0.00%> (-0.20%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1660b1a...3cd8618. Read the comment docs.

[coveralls]

Coverage decreased (-0.1%) to 96.149% when pulling 468397bcde5e4661d47236f1f6e0cca0c23d313a on effigies:enh/norm_units into b2c0f81584efff9c3244bce7874656fd5a7e351b on nipy:master.

[coveralls]

Coverage decreased (-0.1%) to 96.149% when pulling c112e26e1d6fb36b5724c1c10a1426e0f1ae15c7 on effigies:enh/norm_units into b1bf5e76d1c04a4c5b1ebc36f15d01f045d8c4a9 on nipy:master.

[coveralls]

Coverage decreased (-0.1%) to 96.212% when pulling 925d5f93362fb47ad671fa7bbea7fc20ae794804 on effigies:enh/norm_units into b646d5f384a8e39b52dd06de026beee8203b1f82 on nipy:master.

[coveralls]

Coverage decreased (-0.1%) to 96.212% when pulling 976eace09f1930f7171e582e3d1eb53917be9964 on effigies:enh/norm_units into b646d5f384a8e39b52dd06de026beee8203b1f82 on nipy:master.

[coveralls]

Coverage increased (+0.02%) to 96.349% when pulling 1da0097bca4ce04ca992057a8d13669bd62c86e7 on effigies:enh/norm_units into b8545ef665e5a045d67ff9c2af9e677bddbe0962 on nipy:master.

[coveralls]

Coverage increased (+0.02%) to 96.349% when pulling dcc71f11453615130e3b5103015dfa544b6667c0 on effigies:enh/norm_units into b8545ef665e5a045d67ff9c2af9e677bddbe0962 on nipy:master.

[effigies]

This is ready for review. All behavior tested.

[effigies]

Just a bump on this, now the holidays are over.

[coveralls]

Coverage increased (+0.05%) to 93.407% when pulling e56e6d77362d5323ab5a584d37e0ea0ceec3451d on effigies:enh/norm_units into 636d5d2f7e907b721d01b4ddbe96c6eb9b7af3a1 on nipy:master.

[matthew-brett]

I hate to say it - but would you mind putting a test into test_image_api.py?

I'm suffering slightly about this one, because we otherwhere make our outputs canonical - for example, we make our affines point to RAS+ space, for DICOM. So it seems kindve odd to let the formats do whatever with the zooms, and fix it with this function.

I guess it's too late to switch nifti / freesurfer to (mm, mm, mm, seconds) ?

How about get_canonical_zooms?

[matthew-brett]

Or a flag to get_zooms ?

[effigies]

Not too late for MGHHeader, since we haven't released with TR as zooms[3], but yeah, NIFTI uses raw zooms without correcting for units. Unless we want to treat that as a bug, but I think people will be depending on it.

We could deprecate get_zooms, or switch to a keyword argument to specify raw or canonical.

Finally, I'm okay with get_canonical_zooms, but it is getting long. Does it make sense to move to a canonical zooms property? Or even header.zooms that behaves canonically, combined with a get/set_zooms deprecation that warns of new semantics.

And sure, I'll add tests.

[matthew-brett]

I believe I avoided using a property on the basis that the setter would also modify the header (at least, it does now). So, it would break the property manifesto : https://github.com/nipy/nibabel/wiki/property_manifesto

I could imagine another model, where we left the header unmodified from its load state, and stored things like zooms etc inside the image, but that would be an API break.

Could you say more about what people are depending on? Do you think that some people currently have meters set in their header, but are using the units as mm (for example)?

[effigies]

Okay. So no property. I'd be okay with doing the option to get_zooms, if that seems cleaner. We'd presumably want a deprecation schedule to move from a default of raw to default of canonical.

And an example of depending on current behavior:

zooms = np.array(reoriented.header.get_zooms()[:3])
xyz_unit = img.header.get_xyzt_units()[0]
atol = {'meter': 1e-5, 'mm': 0.01, 'micron': 10}[xyz_unit]
rescale = not np.allclose(zooms, target_zooms, atol=atol)

(Adapted from https://github.com/poldracklab/fmriprep/blob/682b507c141254a51316b2367f9834708eb4409e/fmriprep/interfaces/images.py#L236-L256)

Here we took into account the fact that the zooms may not be in mm. If the zooms start being mm but get_xyzt_units still says they're in meters, then we'll be doing the wrong thing.

[matthew-brett]

Right - but I guess the snippet leaves open whether anyone actually has micron etc nifti images ...

A flag seems like the right way to go. I think we should also default Freesurfer to using seconds - what do you think?

[effigies]

Yes, that's true. I've seen someone say they have ms TR images (they ran into issues with an FSL tool that assumed sec), but I don't know if I've ever heard of a meter image in the wild.

So how about a units parameter to get_zooms. I'd be inclined to make the options 'raw' and 'canonical', given this discussion, but open to alternatives. We can default to canonical on any image type known to only allow mm/s, and FreeSurfer, which we'll coerce.

For nifti and any others where we are not guaranteed that raw == canonical, we should default to None, which will provide raw results but warn that the default will switch to canonical (version 4)?

Do you know MINC units off the top of your head? I think I had a look but couldn't find them.

[effigies]

Does bring up a question of set_zooms. What's the appropriate analog of the units flag there?

[effigies]

A thing I just thought about: If the zooms are in microns, then the affine is presumably from VOX -> RAS (um), right? Do we need to account for that at all?

[effigies]

@matthew-brett I have some unpushed changes in the pipeline*, but if you have a minute to give your thoughts on the API issues I brought up in the last three comments, I'd appreciate it.

* Moving to get_zooms(units='raw'|'canonical'); holdup is in trying to prevent a huge number of FutureWarnings littering the test outputs, and even more so building up a will to dig into warnings context managers.

[effigies]

This is ready for review, though there's one major thing to resolve with the tests (see comment).

I've moved to units='norm' instead of units='canonical' for brevity, and added it to set_zooms.

[effigies]

Tests passing, and full coverage for new code.

[effigies]

Just a bump on this one, which is ready for review.

[effigies]

@yarikoptic @matthew-brett Can we go ahead and argue this out a little bit?

When adding an argument for .get_zooms(), the default behavior needs to be the existing behavior, to avoid breaking things that were correctly written. For NIFTI, that means raw zooms. However, we want the default to become normalized (mm/s) in future versions (3.0?), which means we need a period of bugging users/downstream devs so that they don't get stung by the change. But because users cannot change to setting units='norm' or similar until this change goes in, immediately warning will make it very difficult for downstream libraries to support multiple versions of nibabel without annoying users.

In addition, the recently augmented MGHImage has TRs in ms units, which will be surprising to pretty much anybody who expects zooms to be consistent across images.

So the NIFTI/MGH images seem to demand opposite defaults: raw/normalized, respectively. Now, given that we'll be annoying users, is it sensible to go ahead and set the default to units='norm' for all images, and just put up a UserWarning (or something that will be filtered to display only once) noting that the behavior has changed?

I'm not coming to a very satisfying conclusion in any direction, and starting to consider whether we might want a new normalized-zoom getter method after all, but before I convince myself to try something like that, I want your input. What's the least bad way to go here?

[nibotmi]

:umbrella: The latest upstream changes (presumably #630) made this pull request unmergeable. Please resolve the merge conflicts.

[nibotmi]

:umbrella: The latest upstream changes (presumably #550) made this pull request unmergeable. Please resolve the merge conflicts.

[effigies]

Just a bump on this. @yarikoptic @matthew-brett I'm not sure how best to resolve the issues brought up in https://github.com/nipy/nibabel/pull/567#issuecomment-386336861.

[effigies]

Rethinking this API with an eye to consistency, I have an alternative approach:

class SpatialHeader:
    def _get_raw_zooms(self):
        # old get_zooms goes here

    def _get_zoom_units(self):
        # Return units in the same order as zooms, one per zoom
        # Simple default:
        return tuple('unknown' for _ in self._get_raw_zooms())
        # Hard-coded formats can return a static tuple

    def get_zooms(self, *, units=None):
        raw_zooms = self._get_raw_zooms()
        if units is None:
            return raw_zooms
        # units is tuple of (spatial, temporal, other)
        zoom_units = self._get_zoom_units()
        return _rescale(raw_zooms, zoom_units, units)

For the units parameter, I think we should make the inputs simple for the very common cases where we have 3D images or 4D time series. For instance, I think both of the following should work intuitively:

img.get_zooms(units='mm')           # Rescale spatial zooms only
img.get_zooms(units=('mm', 'sec'))  # Rescale spatial and temporal zooms

For less common cases, more complex specifications seem acceptable. One option to make sure that all possibilities can be specified would be to accept a units tuple with a unit per dimension.