refactor: extract the tail repeated in nine apply_transform methods - #63
Open
Hendrik-code wants to merge 1 commit into
Open
refactor: extract the tail repeated in nine apply_transform methods#63Hendrik-code wants to merge 1 commit into
Hendrik-code wants to merge 1 commit into
Conversation
gpu/contrast.py ended nine of its apply_transform loops with the same three things: capture the per-sample mean/std when retain_stats is set, restore them afterwards, then run region selection and drop the channel if the result went non-finite. Written out in full every time. That is _channel_stats, _restore_stats and _select_and_check now: -173 lines, +92. The per-transform loop structure is deliberately left alone rather than inverted into a callback. Every draw in this file happens inside those loops, so keeping them means the RNG consumption order is unchanged by construction, which is what makes the claim below checkable. Verified byte-for-byte rather than by inspection: pushing a fixed volume through all 24 shipped GPU configs under a fixed seed and hashing the output gives identical digests before and after. The test suite would not have caught a reordered draw; this does. _select_and_check takes the transform rather than its three region attributes. All nine call sites read exactly self.in_seg, self.out_seg and self.mix_in_out, and spelling them out as keyword arguments made the call longer than the code it replaced -- the first attempt at this was +224/-174, which is not a simplification. The trainer tail (the same nnU-Net transform sequence in eight get_training_transforms methods) is left for the next commit: nnunetv2 is an optional extra and is not installed in CI, so that one cannot be verified the same way and deserves its own review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Stacked on #62.
gpu/contrast.pyended nine of itsapply_transformloops with the same three things, written out in full every time:retain_statsis set,Those are
_channel_stats,_restore_statsand_select_and_checknow. −173 / +92.How I know it changes nothing
Not by inspection. I pushed a fixed volume through all 24 shipped GPU configs under a fixed seed and hashed the output, before and after:
That matters more than the test suite passing here: the suite would not notice a reordered RNG draw, which is the realistic way to break this refactor. The per-transform loop structure is deliberately left alone rather than inverted into a callback — every draw in this file happens inside those loops, so keeping them means the consumption order is unchanged by construction.
A note on the helper signature
_select_and_checktakes the transform itself, not its three region attributes. My first attempt spelled them out as keyword arguments and came to +224 / −174 — longer than the code it replaced, which is not a simplification. All nine call sites read exactlyself.in_seg,self.out_segandself.mix_in_out, so passingselfis both shorter and more honest about the coupling that already existed.Scope
The trainer tail — the same nnU-Net transform sequence repeated across eight
get_training_transformsmethods — is deliberately not in this PR.nnunetv2is an optional extra and is not installed in CI, so that extraction cannot be verified the same way and deserves its own review. It's the next PR in the stack.Testing
Five new tests in
test_region_and_stats.pypin the extracted helpers directly (per-sample stats, round-tripping mean/std through_restore_stats, and_select_and_checkrejecting a non-finite result while passing a finite one through).Full suite: 105 passed, 200 subtests.
ruffandmypyclean.