Repository navigation
Conversation
resample_image's multi-component branch reassigned its own `image` parameter inside the per-channel loop, so by the time channels_first=image.channels_first ran after the loop, `image` was the last split-channel component, which split_channels() always constructs with channels_first=False. The explicit propagation looked correct but was always a no-op. smooth_image's multi-component branch had the same loop-variable shadowing, plus didn't even attempt to pass channels_first to merge_channels. ANTsImage.__getitem__'s has_components branch had the same omission. Fixed by capturing channels_first before the loop (resample_image, smooth_image) and renaming the loop variable so it can no longer shadow the function's own `image` parameter, and by passing channels_first=self.channels_first through in __getitem__. Verified live: merging two images into a channels_first=True vector image, then round-tripping through resample_image and smooth_image, now preserves channels_first=True on the output (previously silently flipped to False in both). This is a follow-up to ANTsX#938, whose description explicitly reserves "independent channels_first propagation defects in resample_image, smooth_image, and multicomponent indexing" for later work. Signed-off-by: Siddhardha Nanda <99672439+SID-6921@users.noreply.github.com>
This branch has not been deployed
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.
Description
Follow-up to #938, whose description explicitly reserves "independent
channels_firstpropagation defects inresample_image,smooth_image, and multicomponent indexing" for later work.ants.resample_image's multi-component branch reassigns its ownimageparameter inside the per-channel loop (for image in images:). By the timechannels_first=image.channels_firstruns after the loop,imagerefers to the last split-channel component, whichsplit_channels()always constructs withchannels_first=False(theANTsImage.__init__default). The explicit propagation code looks correct but is always a no-op.ants.smooth_image's multi-component branch has the exact same loop-variable shadowing (for image in imagelist:), and on top of that didn't even attempt to passchannels_firsttomerge_channelsat all.ANTsImage.__getitem__'shas_componentsbranch had the same omission assmooth_image— nochannels_firstpassed through.Fix
resample_image: capturechannels_first = image.channels_firstbefore the loop, and rename the loop variable so it can no longer shadow the function's ownimageparameter.smooth_image: same pattern — capture before the loop, rename the loop variable, and passchannels_firstthrough tomerge_channels(previously missing entirely).ANTsImage.__getitem__: passchannels_first=self.channels_firstthrough tomerge_channels.Testing
Verified live against the real compiled
antspackage (pip install antspyx, patching in the fixed pure-Python files):Worth noting: my first attempt at the
smooth_imagefix (just addingchannels_first=image.channels_firstto the existingmerge_channelscall, matching the pattern suggested when this was scoped out in #938's review) was still broken by the same loop-shadowing bug — caught by this live check, which is why the fix renames the loop variable there too rather than only adding the keyword argument.I did not have a way to exercise
ANTsImage.__getitem__'shas_componentsbranch through the pip-installed wheel in the time available, but the change there is the same pattern (no loop-variable shadowing risk, since it's a list comprehension usingself.channels_firstdirectly, not a loop variable).AI Usage Disclosure
This PR was put together with AI assistance (Claude Code). Every change was traced against the real source, verified live against the installed package as shown above, and reviewed by me before pushing.