Preserve lazy cropped/padded backend when deep-copying images#1479
Merged
Conversation
`Image.__deepcopy__` rebuilt path/store-backed images from their source only, discarding `self._backend`. For a lazily cropped or padded image (where `CropOrPad` installs a `_CroppedBackend` / `_PaddedBackend` and leaves `_data` as None), the copy reverted to the full-resolution image. Because `Transform.forward` deep-copies its input, this meant any transform applied after a lazy `CropOrPad` operated on the original, un-cropped image (e.g. `Resample` computed the output grid from the wrong shape). Preserve the derived lazy backend by deep-copying `self._backend` in the source-based branches when `_data` is None, so the crop/pad survives the copy. Add regression tests covering deep-copy of cropped and padded lazy images and a transform applied after a lazy crop.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a correctness issue where copy.deepcopy(Image) could discard a derived lazy backend (e.g., _CroppedBackend / _PaddedBackend from CropOrPad) and silently revert to the original, uncropped/padded source when subsequent transforms deep-copy their inputs.
Changes:
- Preserve an existing lazy backend during
Image.__deepcopy__()when the image is source-backed and still unloaded (_data is None). - Add regression tests covering deepcopy of lazily cropped/padded images and a “transform-after-lazy-crop” pipeline.
- Expand inline documentation in
Image.__deepcopy__()explaining why the backend must be preserved.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/torchio/data/image.py |
Updates Image.__deepcopy__ to preserve lazy derived backends for unloaded, source-backed images. |
tests/test_crop_or_pad.py |
Adds regression tests ensuring lazy crop/pad backends survive deepcopy and are respected by subsequent transforms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use copy.copy instead of copy.deepcopy for the preserved lazy backend in Image.__deepcopy__: the backend is a read-only access adapter, so a shallow copy preserves the derived crop/pad view without deep-copying nibabel/zarr internals or duplicating open I/O resources. - Assert the padded lazy image stays unloaded before and after deepcopy so the test exercises the lazy-backend code path.
Contributor
📖 Docs PreviewPreview of the documentation for this PR: 🔗 https://smokeshow.helpmanual.io/6q0a642115225t380d5a/ Built from 3ea394d |
A backend-only image (e.g. built from a nib.Nifti1Image, with _backend set but no _path or _data) fell through __deepcopy__ to the empty fallback branch, so the copy lost its backend and failed on shape/load. Preserve the backend (shallow copy) in the fallback branch too, and add a regression test for deep-copying a nibabel-backed lazy image.
Assert the intermediate cropped image is unloaded so the regression test reliably exercises the lazy-backend deepcopy path.
fepegar
changed the title
Extract the repeated source-construction and payload-restoration logic into `_deepcopy_from_source`, so `__deepcopy__` drops below the Xenon complexity threshold. Behavior is unchanged.
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.
[Generated by a coding agent]
Summary
Fixes a correctness bug where applying a transform after a lazy
CropOrPadsilently operated on the original, un-cropped image.Root cause
CropOrPadon an unloaded (path-backed) image installs a lazy_CroppedBackend/_PaddedBackendon a newImage, leaving_dataasNoneand keeping the original_path.Image.__deepcopy__rebuilt such images from_pathonly, discardingself._backend(the comment even read "Backend will be lazily recreated from path when needed"). The copy therefore reverted to the full-resolution image.Because
Transform.forwarddeep-copies its input (copy=Trueby default), any transform applied after a lazy crop saw the original shape:Fix
In the source-based branches of
Image.__deepcopy__(_remote_zarr_uri,_path,_zarr_store), when_data is Nonebut a backend is set, deep-copyself._backendso a derived lazy view (cropped/padded) survives the copy. Loaded/in-memory images are unaffected (they still clone_data).After the fix:
Tests
Added regression tests in
tests/test_crop_or_pad.py::TestLazyBackends:CropOrPad(10)thenPad(2)->14^3).uv run pytest tests/test_crop_or_pad.py tests/test_image.py tests/test_backends.py tests/test_transforms_base.py tests/test_crop.py-> all passing.ruffandtyclean.Notes
This bug was discovered while building a demo for #1477 (partial-volume label resampling); this PR is independent of that change and targets
main.