Apply the defacing mask to every volume of a 4D image - #84
Open
Arthur031221 wants to merge 1 commit into
Open
Conversation
`warped_mask_data * n` multiplies the mask array by the number of volumes instead of repeating it, so `np.stack` iterated the 3D mask as a sequence of 2D slices. The stacked array came out transposed, (X, Y, Z) turned into (Y, Z, X), and filled with n rather than 1. Defacing a multi-volume image then fails with a ValueError raised from inside the except branch that was meant to handle it. Where the first axis happens to equal the volume count the wrong array still broadcasts, and the image is written with the mask applied along permuted axes instead. Restore the list repetition, `[warped_mask_data] * n`, in `deface_image` and in the `--applyto` branch, and add a regression test covering both geometries.
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.
utils.py:132builds the 4D mask withwarped_mask_data * nis a scalar multiplication, so it gives one 3D array fullof
nrather thanncopies of the mask.np.stackthen iterates that array asa sequence of 2D slices, so the result comes out 3D with the axes rolled,
(X, Y, Z)becoming(Y, Z, X), holdingnwhere the mask held 1.The brackets went missing in 2c6e781 ("Fix nibabel
.get_datawarnings", July2022), which rewrote
get_data()todataobj.__main__.py:142carries thesame expression and took the same change.
git tag --contains 2c6e781givesv2.0.1, v2.0.2 and v2.1.0; of those, PyPI carries 2.0.2 and 2.1.0, so every
release installable from PyPI since 2.0.2 ships the broken form, including the
current 2.1.0.
The
exceptbranch is from #17 ("Make pydeface work with multivolumeanatomicals", 2018), which had an axis problem of its own: its first commit wrote
np.array([mask] * n), volumes on the leading axis, and its last commit d2668e3("fix axes ordering") changed that to
np.stack([mask] * n, axis=-1)to put themlast. Measured with a
(6, 7, 8)mask and 3 volumes:So 2c6e781 didn't reintroduce the old leading-axis mistake. It produced a third
shape matching neither, one rank down, and the branch stopped working at all.
On current
master, defacing a 4D image, with FLIRT stubbed so no FSL is needed:The second failure comes from inside the handler meant to catch the first.
Where the first axis happens to equal the volume count, the wrong array still
broadcasts and nothing is raised. On a
(3, 4, 5, 3)input the voxel the maskzeroes comes out
[0, 6, 9]across the three volumes instead of[0, 0, 0], itsinput value having been
[1, 2, 3], and a voxel the mask keeps goes from[79, 80, 81]to[237, 240, 243]. Everything that survives is multiplied by thevolume count.
Restoring the list repetition fixes both. I kept this to putting back what
2c6e781 dropped rather than rewriting the branch.
warped_mask_data[..., np.newaxis]gives a bit-identical result without materialising the copies and isthe better expression, but that is a change to the branch rather than a repair of
it, so I left it alone.
The regression test is parametrized over both geometries and checks that the
masked voxel is zero in every volume while every other voxel keeps its original
value. It stubs FLIRT rather than gating on FSL, for two reasons. The assertions
need a mask with known contents, and real FLIRT registration on a tiny synthetic
image wouldn't give one. And a test gated the way
test_deface_imageis gatedwould run in exactly one place: the Actions matrix installs no FSL, so it would
skip in all six cells there, and only the CircleCI docker image, whose pixi dev
environment carries
fsl-flirtand thefsllauncher fromfsl-misc_tcl, wouldexercise it. The stub runs everywhere, CircleCI included.
The remaining skip is
test_deface_image.ruff check .andruff format --check .are clean on 0.15.12.Three things to flag. The
--applytoline in__main__.pyis the identicalexpression broken by the same commit and is fixed here too, but the test drives
deface_imageonly;__main__.pyhas no coverage in the suite today, andwriting its first test seemed out of proportion to a two line fix. I ran this on
Linux with Python 3.12.3 and numpy 2.5.1, a single cell of the matrix. And I have
one other open item at this repo, #82, on the FSL presence check in
deface_image; this fix is independent of it, though the test'sshutil.whichstub does sidestep the check that issue describes.