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.

nibabel/nibabel/freesurfer/mghformat.py

Lines 240 to 246 in b8545ef

	def get_zooms(self):
	''' Get zooms from header
	Returns the spacing of voxels in the x, y, and z dimensions.
	For four-dimensional files, a fourth zoom is included, equal to the
	repetition time (TR) in ms (see `The MGH/MGZ Volume Format
	<mghformat>`_).

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 2bfc8d7 on effigies:enh/norm_units into b2c0f81 on nipy:master.

[codecov-io]

Codecov Report

Base: 92.40% // Head: 92.18% // Decreases project coverage by -0.21% ⚠️

Coverage data is based on head (98e43a0) compared to base (943e5e8).
Patch coverage: 65.59% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
- Coverage   92.40%   92.18%   -0.22%     
==========================================
  Files          98       98              
  Lines       12248    12326      +78     
  Branches     2516     2546      +30     
==========================================
+ Hits        11318    11363      +45     
- Misses        611      630      +19     
- Partials      319      333      +14     

Impacted Files	Coverage Δ
nibabel/spatialimages.py	94.30% <37.50%> (-2.00%)	⬇️
nibabel/analyze.py	97.48% <42.85%> (-1.11%)	⬇️
nibabel/freesurfer/mghformat.py	92.94% <62.50%> (-2.13%)	⬇️
nibabel/nifti1.py	90.97% <69.64%> (-1.66%)	⬇️
nibabel/ecat.py	88.25% <100.00%> (+0.06%)	⬆️
nibabel/minc1.py	90.36% <100.00%> (+0.11%)	⬆️
nibabel/viewers.py	95.49% <0.00%> (-0.65%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

Coverage increased (+0.02%) to 96.349% when pulling dcc71f1 on effigies:enh/norm_units into b8545ef 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 e56e6d7 on effigies:enh/norm_units into 636d5d2 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.