Skip to content

Apply the defacing mask to every volume of a 4D image - #84

Open
Arthur031221 wants to merge 1 commit into
poldracklab:masterfrom
Arthur031221:fix-multivolume-mask-stack
Open

Apply the defacing mask to every volume of a 4D image#84
Arthur031221 wants to merge 1 commit into
poldracklab:masterfrom
Arthur031221:fix-multivolume-mask-stack

Conversation

@Arthur031221

Copy link
Copy Markdown

utils.py:132 builds the 4D mask with

tmpdata = np.stack(warped_mask_data * infile_img.shape[-1], axis=-1)

warped_mask_data * n is a scalar multiplication, so it gives one 3D array full
of n rather than n copies of the mask. np.stack then iterates that array as
a sequence of 2D slices, so the result comes out 3D with the axes rolled, (X, Y, Z) becoming (Y, Z, X), holding n where the mask held 1.

The brackets went missing in 2c6e781 ("Fix nibabel .get_data warnings", July
2022), which rewrote get_data() to dataobj. __main__.py:142 carries the
same expression and took the same change. git tag --contains 2c6e781 gives
v2.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 except branch is from #17 ("Make pydeface work with multivolume
anatomicals", 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 them
last. Measured with a (6, 7, 8) mask and 3 volumes:

np.array([mask] * 3)           # (3, 6, 7, 8)  PR #17 before d2668e33
np.stack([mask] * 3, axis=-1)  # (6, 7, 8, 3)  d2668e33, correct
np.stack(mask * 3, axis=-1)    # (7, 8, 6)     after 2c6e781, every value 3

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:

Traceback (most recent call last):
  File "pydeface/utils.py", line 130, in deface_image
    outdata = infile_data.squeeze() * warped_mask_data
              ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
ValueError: operands could not be broadcast together with shapes (6,7,8,3) (6,7,8)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "pydeface/utils.py", line 133, in deface_image
    outdata = infile_data * tmpdata
              ~~~~~~~~~~~~^~~~~~~~~
ValueError: operands could not be broadcast together with shapes (6,7,8,3) (7,8,6)

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 mask
zeroes comes out [0, 6, 9] across the three volumes instead of [0, 0, 0], its
input 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 the
volume 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 is
the 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_image is gated
would 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-flirt and the fsl launcher from fsl-misc_tcl, would
exercise it. The stub runs everywhere, CircleCI included.

python -m pytest tests/ -q

  24919aa with the new test only    2 failed, 3 passed, 1 skipped
  with this change                  5 passed, 1 skipped

The remaining skip is test_deface_image. ruff check . and
ruff format --check . are clean on 0.15.12.

Three things to flag. The --applyto line in __main__.py is the identical
expression broken by the same commit and is fixed here too, but the test drives
deface_image only; __main__.py has no coverage in the suite today, and
writing 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's shutil.which
stub does sidestep the check that issue describes.

`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant