Coverage decreased (-0.1%) to 96.149% when pulling2bfc8d7 on effigies:enh/norm_units intob2c0f81 on nipy:master.

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.

@@            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

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

Coverage decreased (-0.1%) to 96.149% when pulling468397b on effigies:enh/norm_units intob2c0f81 on nipy:master.

Coverage decreased (-0.1%) to 96.149% when pullingc112e26 on effigies:enh/norm_units intob1bf5e7 on nipy:master.

Coverage decreased (-0.1%) to 96.212% when pulling925d5f9 on effigies:enh/norm_units intob646d5f on nipy:master.

Coverage decreased (-0.1%) to 96.212% when pulling976eace on effigies:enh/norm_units intob646d5f on nipy:master.

Coverage increased (+0.02%) to 96.349% when pulling1da0097 on effigies:enh/norm_units intob8545ef on nipy:master.

Coverage increased (+0.02%) to 96.349% when pullingdcc71f1 on effigies:enh/norm_units intob8545ef on nipy:master.

This is ready for review. All behavior tested.

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

Coverage increased (+0.05%) to 93.407% when pullinge56e6d7 on effigies:enh/norm_units into636d5d2 on nipy:master.

I hate to say it - but would you mind putting a test intotest_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 aboutget_canonical_zooms?

Or a flag toget_zooms ?

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.

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)?

Okay. So no property. I'd be okay with doing the option toget_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=notnp.allclose(zooms,target_zooms,atol=atol)

(Adapted fromhttps://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 butget_xyzt_units still says they're in meters, then we'll be doing the wrong thing.

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?

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 aunits parameter toget_zooms. I'd be inclined to make the options'raw' and'canonical', given this discussion, but open to alternatives. We can default tocanonical on any image type known to only allowmm/s, and FreeSurfer, which we'll coerce.

For nifti and any others where we are not guaranteed thatraw == canonical, we should default toNone, 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.

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

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?

@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 toget_zooms(units='raw'|'canonical'); holdup is in trying to prevent a huge number ofFutureWarnings littering the test outputs, and even more so building up a will to dig intowarnings context managers.