Write .mgz files with the right data type - #874
Merged
Conversation
MGHHeader.from_header returns float32 whatever the source header says, so the type of an .mgz depended on whether its header came from a .nii or a .mgz. The 1mm copy CerebNet conforms for a high-res subject was written as float32 at four times the size, holding only integers, although uint8 was asked for. as_mgh_image now carries the type across as it already does the fov, and warns rather than aborting for a type MGH cannot store.
reduce_to_aseg_and_save kept the int16 of the segmentation it reduces, so aseg.auto.mgz and the aseg.presurf files derived from it became int16, where FreeSurfer and FastSurfer up to v2.3.3 write uchar. The __main__ path of the same module already narrowed; this one did not. It narrows only integer labels within 0 to 255, so a float is not rounded and a negative label is not clipped.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Headerless unsupported types can still abort, and inherited integer headers can corrupt floating-point soft-label outputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Preserves intended MGZ data types and restores lossless uint8 output for aseg files.
Changes:
- Carries source-header data types into MGH images.
- Narrows eligible aseg labels to
uint8. - Adds dtype regression coverage.
File summaries
| File | Description |
|---|---|
FastSurferCNN/data_loader/data_utils.py |
Preserves dtype during MGH conversion. |
FastSurferCNN/reduce_to_aseg.py |
Narrows lossless aseg data to uint8. |
test/image/test_mgh_dtype.py |
Tests conversion and narrowing behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Carrying the header type across covered a case it should not: an integer header applied to floating-point data rounded it away, which would have stored the CC soft labels as uchar on the NIfTI path, where nibabel had left them float32. It is now only carried where nothing is lost. Headerless images are also built with a default header first, since nibabel raises for a type MGH cannot store while constructing the image, before the fallback could run.
The CC soft labels are written with the header of the conformed image, which is uchar, so a probability of 0.37 was stored as 0 and the map came back as a binary mask with two distinct values instead of 28660. Floating-point data is now never stored as an integer type whatever the header says, which also covers the MGH case, where nibabel applies the type while constructing the image rather than afterwards. Only the type is overridden; the rest of the header is carried through.
as_mgh_image applied the header's integer type without checking the data fits it, so int16 labels written with a uchar header clipped 500 to 255, silently. On the NIfTI path that was a regression: the type used to be dropped entirely, which preserved the value by accident. The fit test moves into a shared fits_dtype, so the aseg and mask writers narrow by the same rule instead of each carrying their own, and reduce_to_aseg no longer mutates the header it borrowed to say the same thing.
The guard against clipping widened to the array's own type, but MGH has no int64, so the fallback returned to the narrow header and clipped after all: int64 labels holding 500 were written as 255, after a log line claiming clipping had been avoided. It now picks the narrowest storable type that fits, uint16 there. The three callers that wanted uchar say so with prefer_dtype instead of each repeating the fit test, and save_image warns when the type it is told to force loses data.
…r writes The choice was four inline branches that read the array's range once per candidate type, and it could land on uint16, which FreeSurfer reads but never writes itself. choose_dtype now holds the whole decision, reads the range once, and falls back only to uchar, int16 or int32; a header asking for uint16 is still honoured. The widening test asserts that the values survive and the type is storable rather than naming the preferred one, which is pinned once in the new unit test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Decide the data type once, and refuse one that cannot be honoured
Four outputs came out with the wrong data type. Found by comparing a released v2.5.4 run against the
v2.3.3 reference and against what FreeSurfer writes, then tracing each one to the writer.
The fixes are small. Most of this branch is the rule that replaces them, because every one of the
four was a different symptom of the same missing decision: nothing owned the question "what type
does this file get".
The rule
save_imageandas_mgh_imagenow decide the type in one place,choose_dtype:dtype, or else what the header carries.Narrowing is the caller's to do, deliberately and outside, because only the caller knows whether
to cast, to rescale or to refuse.
the type is applied, so a wrong list on our side cannot cause a wrong refusal.
Nothing is substituted and nothing is guessed. The one output whose type is genuinely not ours to
choose, the archival copy of the input in
mri/orig/001.mgz, asks forstorable_dtypeexplicitly.The rule is the same for every format. The file name only decides which types are available at all:
MGH has no int64 and no float64, NIfTI has both.
.mgztargetThe same table for a
.nii.gztarget has the int64 and float64 columns filled in rather thanrefusing. Every cell is either the requested type or a refusal; nothing warns.
What was wrong
A header from a NIfTI lost the type
MGHHeader.from_headerreturns float32 whatever the source header says, so the written typedepended on which container the header came from:
>f4The 1 mm copy CerebNet conforms for a high-res subject,
orig.10mm.mgz, was stored as float32 atfour times the size holding only integers, although uint8 was asked for and honoured all the way
down to the write. This is the same nibabel behaviour as the
fov=0bug in #873 on a differentfield, so the fix sits in the same function.
The aseg files were int16 instead of uchar
reduce_to_aseg_and_savekept the type of the segmentation it reduces:aseg.auto_noCCseg.mgzaseg.auto.mgz>i2aseg.presurf.mgz>i2aseg.auto.mgzaseg.presurf.hypos.mgz>i2aseg.auto.mgzThe module's own
__main__path already narrowed, which is whyaseg.auto_noCCseg.mgzstayeduint8; the library path used by
paint_cc_into_preddid not. The twopresurffiles are derivedfrom
aseg.auto.mgzby FreeSurfer, so they are expected to follow rather than measured to; therefreshed quicktest reference will confirm it.
Both call sites now ask for uchar explicitly. An aseg has no label outside 0 to 255, so a label that
does not fit means something went wrong upstream and now says so instead of being written as a type
FreeSurfer does not expect.
Probabilities were stored as uchar
The corpus callosum soft labels are softmax outputs written with the header of the conformed image.
That header is uchar, so every value below 0.5 became 0 and the probability map came back as a
binary mask.
callosum.CC.soft.mgzand its two siblings now carry real probabilities.The call site says
dtype=np.float32, which is where that decision belongs. Any writer that handsfloat data to an integer type now raises rather than rounding it away.
The hypothalamus mask was float32 instead of uchar
hypothalamus_mask.HypVINN.nii.gzA copy-paste slip in the reorientation rework: the mask block set uchar on its header and then
overwrote it with the float32 line from the logits block two functions down. The prediction block
next to it got the right type, which is why only the mask was affected.
Also in this branch
save_imagereturns the image it wrote.load_maybe_conformbuilt a second one, so the typewas decided twice and a
.nii.gzconf_namewas handed back as anMGHImagethat did not matchthe file on disk.
scale factor, and a label read back through one is no longer the integer it was written as. In
practice this is belt and braces, since the refusal rule prevents the case that would trigger it.
save_imageinstead of assembling headers by hand. Two ofthe four bugs above lived in that function. Verified byte-identical to the previous code path on
the same inputs, so this is a simplification with no output change.
save_logitscalled a method nibabel does not have.Nifti1Header.set_data_typedoes notexist, so the function raised
AttributeErroron its first line. Kept rather than removed, sinceit is a debugging aid.
headeris now required inas_mgh_image. Every image we write is derived from one we read,and the header is the only carrier of the acquisition parameters and of the type.
asserton the output extension is gone. It could not fire without theelsebranch below also firing, and it was validating input with an
assert, which vanishes underpython -O.Two commits at the end are unrelated to data types: they write the comment and dash conventions into
CONTRIBUTING.mdand fix a code fence that had drifted out of the numbered list.Impact
Six files per subject change type, four of them back to what v2.3.3 and FreeSurfer write:
aseg.auto.mgzand the twopresurffiles>i2hypothalamus_mask.HypVINN.nii.gzorig.10mm.mgz, high-res subjects only>f4callosum.CC.soft.mgzand its two siblings>f4aseg.mgz,aparc+aseg.mgzandwmparc.mgzalready matched FreeSurfer and are untouched.One measurement changes. The CC soft labels go from a binary mask to real probabilities. That is
the point of the fix, not a side effect. Nothing else changes numerically.
Header bytes and file sizes change on the files above, so the quicktest reference will differ. Every
image the current dev pipeline produces was re-checked against the new rule and none of the 86 is
refused, so no existing run turns into a failure.
Tests
test/image/test_mgh_dtype.py, 49 cases:dtype, in both formatsstorable_dtypekeeping the kind and the signedness, and writing the archival copytr,te,tiandflip_angleproduces a
>f4image that keeps all four fields and the affine, and the caller's header objectis not modified, which matters because the aseg and the mask are written from one header on two
threads
storable_dtypepick something unstorable478 tests pass in
test/imageandtest/config, 534 includingtest/shell/test_bash4_lint.py.Ruff and codespell clean.
Follow-ups, not in this PR
mri/orig/001.mgzis not written at all for.nii.gzinput, a regression since v2.5.4 that isseparate from the type question. The plan is to make it a verbatim copy of the input under the
input's own extension, which loses nothing, needs no type policy, and removes the only caller of
storable_dtype.rawavg.mgzis currently a link to the conformedorig.mgzrather than being built from theinput, because the conform step moved out of
recon-surf.sh. It should be built from001.shipped v2.5.4 image is not reproducible from source, which needs settling first.