Preserve inverse transform image scope - #1481
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug in TorchIO’s invertible transform history: inverse transforms are now reconstructed with the same image scope (include/exclude) as the original forward transform, preventing unintended mutation of untouched images during apply_inverse_transform().
Changes:
- Extend
AppliedTransformhistory records to include the names of images affected by each transform. - Rebuild inverse transforms with an
includelist derived from the recorded affected image names, intersected with the current data’s images. - Add regression tests covering
Gammainclude/exclude scoping andFlipinclude scoping during inversion.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/torchio/transforms/transform.py |
Records affected image names into AppliedTransform during forward passes. |
src/torchio/transforms/inverse.py |
Reapplies recorded image scope when reconstructing inverse transforms; threads current data into inverse construction. |
src/torchio/data/invertible.py |
Passes self into inverse reconstruction so current image names are available. |
tests/test_gamma.py |
Adds regression tests ensuring inverse respects include/exclude scope for Gamma. |
tests/test_flip.py |
Adds regression test ensuring inverse respects include scope for Flip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inverse = instance.inverse(trace.params) | ||
| if trace.image_names is not None and current_image_names is not None: | ||
| matching_names = [ | ||
| name for name in trace.image_names if name in current_image_names | ||
| ] | ||
| if matching_names: | ||
| inverse.include = matching_names | ||
| inverse.exclude = None | ||
| steps.append(inverse) |
There was a problem hiding this comment.
Handled in the latest push. _restrict_inverse_to_trace_images() now returns whether the inverse should be appended; when recorded image names have no match in the current data, that inverse step is skipped instead of falling back to all images. I also added a regression test for the removed-image case and adjusted the copied-history test to require matching image names.
There was a problem hiding this comment.
[Generated by a coding agent]
For the record: the skip-when-no-overlap behavior added in response to this comment is exactly what introduced the TTA regression flagged in the later review (history copied onto a differently-named pred image gets its spatial inverse silently dropped).
Recording the forward include/exclude instead avoids needing a skip at all. The removed-image case you raised here stays safe because _get_images only returns images that exist, so an include naming an absent image is already a no-op.
36f3eac to
7edc7db
Compare
| def _restrict_inverse_to_trace_images( | ||
| inverse: Transform, | ||
| trace: AppliedTransform, | ||
| current_image_names: set[str] | None, | ||
| ) -> bool: | ||
| if trace.image_names is None or current_image_names is None: | ||
| return True | ||
| matching_names = [name for name in trace.image_names if name in current_image_names] | ||
| if not matching_names: | ||
| return False | ||
| inverse.include = matching_names | ||
| inverse.exclude = None | ||
| return True |
There was a problem hiding this comment.
[Generated by a coding agent]
Good catch, and I verified it empirically:
- On
main, the documented TTA snippet (history copied ontopred=..., thenapply_inverse_transform(ignore_intensity=True)) correctly inverts the spatial transform. - On this branch the inverse is silently skipped:
restored.predequals the still-augmented data, because the forward recordedimage_names=['t1']and there is no overlap with{pred}.
On the suggested remedy, though, I'd avoid the intensity/non-intensity split with a "fall back to all current images" branch: that reintroduces the over-application this PR is trying to prevent (for spatial transforms) and is hard to reason about.
A simpler fix that satisfies both your review comments at once: record the forward transform's own include/exclude (not the resolved image names) and set them on the inverse. Then:
- The original
Gamma(include=['a'])corruption is fixed: the inverse getsinclude=['a']. - TTA is preserved: a forward with no scope records
include=None, so the inverse applies to whatever images are present, including a renamedpred. - The removed-image case from the earlier thread stays safe without any explicit skip:
_get_imagesonly returns images that exist, soinclude=['a']on a subject withoutais a natural no-op. - No
data=threading, nocurrent_image_names, no intensity/spatial branching.
Intensity inverses re-apply their own scalar-only filter, so label maps still won't be touched.
fepegar
left a comment
There was a problem hiding this comment.
[Generated by a coding agent]
Thanks for tackling this, the underlying bug (#1480) is real and the new include/exclude regression tests are valuable.
My one blocking concern is a regression in the documented test-time-augmentation workflow, which I verified empirically:
| scenario | main |
this branch |
|---|---|---|
copy history onto a renamed pred image, then apply_inverse_transform |
inverse applied ✅ | inverse silently skipped ❌ |
The TTA guide (docs/how-to/tta.md, lines 37-46) wraps the model output as pred=tio.ScalarImage(...), copies applied_transforms, and inverts. Because the forward Flip recorded image_names=['t1'] and there's no overlap with {pred}, _restrict_inverse_to_trace_images returns False and the spatial inverse is dropped, so predictions are never mapped back to the original space.
Two things mask this in the PR:
tests/test_flip.py::test_inverse_on_image_via_subjectwas changed to renamepred->t1so the names match, which hides the regression rather than addressing it.docs/how-to/tta.mdstill documents the now-brokenpred=...pattern and isn't updated.
Suggested approach
Record the forward transform's own include/exclude in the trace and set them on the inverse, instead of recording resolved image names and intersecting them with the current data. This:
- fixes the original corruption (
Gamma(include=['a'])-> inverseinclude=['a']); - preserves TTA, since a scopeless forward records
include=Noneand the inverse applies to whatever images are present (including a renamedpred); - keeps the removed-image case safe with no explicit skip, because
_get_imagesonly returns images that exist, so anincludenaming an absent image is a no-op; - drops the
data=threading,current_image_names,_get_image_names, and the intensity/spatial special-casing entirely.
Intensity inverses re-apply their own scalar-only filter, so label maps stay untouched either way.
If you'd like, I'm happy to push a commit implementing this and add a TTA regression test (renamed prediction image) plus keep the original pred-named test_inverse_on_image_via_subject.
|
Thanks for the PR, @gaoflow. Would you like to keep addressing the clankers' reviews? Otherwise I'm happy to take over. |
dba4db0 to
e3531f1
Compare
|
I kept going and pushed
Verification: uv run tox -e test -- tests/test_flip.py tests/test_gamma.py tests/test_inverse.py tests/test_pad.py -q
uv run tox -e lint
uv run tox -e format
uv run tox -e prek |
e3531f1 to
16a9175
Compare
|
@all-contributors please add @gaoflow for bug, code. |
|
I've put up a pull request to add @gaoflow! 🎉 |
Summary
Fixes #1480.
Tests
AI assistance was used under my direction.