Copy the input into the subject directory instead of re-encoding it - #875
Open
m-reuter wants to merge 8 commits into
Open
Copy the input into the subject directory instead of re-encoding it#875m-reuter wants to merge 8 commits into
m-reuter wants to merge 8 commits into
Conversation
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.
mri/orig/001.mgzis meant to be a copy of the input, but it was a read-and-write. For a scaledNIfTI, whose array nibabel hands back as float64, MGH could not store the requested type and the
write raised inside a discarded Future, so the file was missing for every
.nii.gzinput withno error anywhere. #874 stopped the crash, which only turned the missing file into a 28.8 MB float32
stand-in for a 23.8 MB int16 input.
The fix
Two files with two jobs, both written by
run_fastsurfer.sh, the only place that knows the input:mri/orig/001.<ext>mri/rawavg.mgz.mgz, converted otherwise.A copy cannot lose anything and needs no data-type policy.
rawavg.mgzhas to keep exactly thatname because
pctsurfconbuilds the path itself, so arawavg.nii.gzwould silently breakgray/white contrast. That is what makes the two files distinct rather than redundant.
Verified on both real quicktest inputs:
001byte-identical in each case,rawavg.mgzconverted to>f4withfov=256.0for the 0.8mm scaled NIfTI and a symlink for the 1.0mm MGH.Why the conversion is ours
The segmentation block has no FreeSurfer dependency, so
mri_convertis out, and both Pythoncandidates drop the field of view that #873 fixed in our writer:
nib-convert>f4neuroreg.image.save_image0.6.2>f4save_image(ours)>f4Voxel values are identical in all three. Switching to neuroreg is a one-line change once the fix on
its main branch ships. This also fixes
fov=0on the T2 copy, which went throughnib-convert.recon-all converts its own
-T2input with--no_scale 1, so keeping the input's values ratherthan rescaling them is also what FreeSurfer does at this step.
Also in this PR
orig.mgzwrite had the same discarded Future. It isnow drained by
mainwith the others, so a failed write reaches the exit code. That mechanism iswhy this bug survived two releases.
mri/orig/T2raw.<ext>for the copyand
mri/orig/T2raw.mgzfor the converted one, which N4 reads. recon-all converts a-T2inputto that path with
--no_scale 1, andsamsegand-T2piallook for it there. For an.mgzT2the two collapse into one file. HypVINN is unaffected: it reads the bias-corrected
mri/T2_nu.mgz, not the raw copy.copy_orig_nameremoved fromrun_prediction.pyandcommon.py, about 35 lines. Twomechanisms writing one path would otherwise race.
run_prediction.pystandalone no longer writes001, which is right for something that is not a pipeline.
recon-surf.shkeeps the rawavg it is given rather than linkingorig.mgzover it, and warnsand falls back to the old behaviour when there is none, so
--surf_onlyon a directory preparedelsewhere does not regress.
CerebNet/config/dataset.pyandEDITING.mdpoint atmri/rawavg.mgz, since001nolonger has a fixed name.
Impact
New per subject:
mri/rawavg.mgzfor--seg_onlyruns. Renamed:mri/orig/001.mgzto001.<ext>, andmri/orig/T2.001.mgztomri/orig/T2raw.<ext>plusmri/orig/T2raw.mgz. The T2names were FastSurfer's own invention and now match recon-all.
rawavg.mgznow holds the input rather than the conformed image, so?h.w-g.pctis sampled fromthe input. The contrast is scale invariant, so this is quantisation noise rather than a shift:
conform's rescale is a pure multiplication, sincesrc_minmeasures 0 on both real inputs, and100*(w-g)/(0.5*(w+g))cancels it exactly. For the 1.0mm case the input is already conformed uchar,so nothing changes. Neither
?h.w-g.pct.statshas a quicktest tolerance yaml, so adding thatcoverage is separate work and not in this PR.
The quicktest reference will differ:
mri/orig/001.*reappears for the 0.8mm case andrawavg.mgzbecomes a real file rather than a link.
Tests
test/image/test_copy_input.py, 9 cases: the copy is byte-identical for a scaled NIfTI and for anMGH input, compound extensions survive so
.nii.gzdoes not become.gz, the converted rawavgcarries the right dtype and fov and preserves values, the MGH rawavg is a relative symlink so
the subject directory stays movable, re-running swaps a symlink for a real file and back, a NIfTI T2
produces both the archive and the conversion while an MGH T2 needs only one file, and a missing
input is reported before anything is written so a typo leaves no half-built directory.
expected-files.yamlgains afile_patternssection, since an exact-set check cannot express a namethat depends on the input, and
test_file_existencechecks those globs.543 tests pass across
test/image,test/configandtest/shell/test_bash4_lint.py. Ruff andcodespell clean, both shell scripts pass
bash -n. The wiring inrun_fastsurfer.shandrecon-surf.shhas no unit coverage, so a quicktest run is the first real check on it.