Skip to content

Preserve inverse transform image scope - #1481

Merged
fepegar merged 4 commits into
TorchIO-project:mainfrom
binggao1230:1480-preserve-inverse-scope
Jun 24, 2026
Merged

Preserve inverse transform image scope#1481
fepegar merged 4 commits into
TorchIO-project:mainfrom
binggao1230:1480-preserve-inverse-scope

Conversation

@binggao1230

Copy link
Copy Markdown
Contributor

Summary

  • record the image names affected by each applied transform
  • restrict rebuilt inverse transforms to those names when they exist on the current data
  • add Gamma include/exclude and Flip include regression coverage

Fixes #1480.

Tests

  • uv run --group test pytest tests/test_gamma.py tests/test_flip.py -q -k 'inverse_respects_include_scope or inverse_respects_exclude_scope'
  • uv run --group test pytest tests/test_gamma.py tests/test_flip.py tests/test_inverse.py tests/test_transforms_base.py -q
  • uv run --group test pytest tests/test_bias_field.py tests/test_normalize.py tests/test_standardize.py tests/test_remap_labels.py tests/test_one_hot.py tests/test_sequential_labels.py tests/test_transpose.py tests/test_crop.py tests/test_pad.py -q
  • uv run --group test pytest -m 'not slow' -q
  • uv run --group quality ruff check
  • uv run --group quality ruff format --check
  • uv run python -m compileall -q src tests
  • uvx --with tox-uv tox -e lint,types,format

AI assistance was used under my direction.

Copilot AI review requested due to automatic review settings June 19, 2026 03:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AppliedTransform history records to include the names of images affected by each transform.
  • Rebuild inverse transforms with an include list derived from the recorded affected image names, intersected with the current data’s images.
  • Add regression tests covering Gamma include/exclude scoping and Flip include 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.

Comment on lines +58 to +66
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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@binggao1230
binggao1230 force-pushed the 1480-preserve-inverse-scope branch 3 times, most recently from 36f3eac to 7edc7db Compare June 20, 2026 07:06
@fepegar
fepegar requested a review from Copilot June 21, 2026 22:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment thread src/torchio/transforms/inverse.py Outdated
Comment on lines +71 to +83
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Generated by a coding agent]


Good catch, and I verified it empirically:

  • On main, the documented TTA snippet (history copied onto pred=..., then apply_inverse_transform(ignore_intensity=True)) correctly inverts the spatial transform.
  • On this branch the inverse is silently skipped: restored.pred equals the still-augmented data, because the forward recorded image_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 gets include=['a'].
  • TTA is preserved: a forward with no scope records include=None, so the inverse applies to whatever images are present, including a renamed pred.
  • The removed-image case from the earlier thread stays safe without any explicit skip: _get_images only returns images that exist, so include=['a'] on a subject without a is a natural no-op.
  • No data= threading, no current_image_names, no intensity/spatial branching.

Intensity inverses re-apply their own scalar-only filter, so label maps still won't be touched.

@fepegar fepegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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_subject was changed to rename pred -> t1 so the names match, which hides the regression rather than addressing it.
  • docs/how-to/tta.md still documents the now-broken pred=... 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']) -> inverse include=['a']);
  • preserves TTA, since a scopeless forward records include=None and the inverse applies to whatever images are present (including a renamed pred);
  • keeps the removed-image case safe with no explicit skip, because _get_images only returns images that exist, so an include naming 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.

@fepegar

fepegar commented Jun 21, 2026

Copy link
Copy Markdown
Member

Thanks for the PR, @gaoflow. Would you like to keep addressing the clankers' reviews? Otherwise I'm happy to take over.

@binggao1230
binggao1230 force-pushed the 1480-preserve-inverse-scope branch from dba4db0 to e3531f1 Compare June 22, 2026 17:32
@binggao1230

binggao1230 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

I kept going and pushed 16a9175b with the suggested approach:

  • AppliedTransform now records the forward transform’s include / exclude scope instead of resolved image names.
  • inverse reconstruction applies that recorded scope to the inverse transform and drops the data= / current-image-name intersection path.
  • restored the copied-history prediction case to use a renamed pred image, so the documented TTA pattern is covered again.

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
@binggao1230
binggao1230 force-pushed the 1480-preserve-inverse-scope branch from e3531f1 to 16a9175 Compare June 22, 2026 17:34
fepegar
fepegar previously approved these changes Jun 24, 2026

@fepegar fepegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Thanks for your contribution, @gaoflow. Thanks also for disclosing your usage of AI.

Comment thread src/torchio/transforms/spatial/crop_or_pad.py Outdated
@fepegar

fepegar commented Jun 24, 2026

Copy link
Copy Markdown
Member

@all-contributors please add @gaoflow for bug, code.

@allcontributors

Copy link
Copy Markdown
Contributor

@fepegar

I've put up a pull request to add @gaoflow! 🎉

@fepegar
fepegar merged commit 4a83e13 into TorchIO-project:main Jun 24, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants