Skip to content

fix: default RandomRedistributeSegGPU's retain_stats to True - #112

Open
Hendrik-code wants to merge 1 commit into
mainfrom
hm/fix-redistribute-retain-stats
Open

Hendrik-code wants to merge 1 commit into
mainfrom
hm/fix-redistribute-retain-stats

Conversation

@Hendrik-code

Copy link
Copy Markdown
Collaborator

The defect

RandomRedistributeSegGPU normalises each sample to [0, 1], adds a perturbation of up to 2.0 in that space — twice the input's full dynamic range — and, when retain_stats=False, never maps the result back.

So the output does not depend on the input's scale at all:

input std   1.0 -> output range (0.52, 2.85) mean 2.25
input std   1.5 -> output range (0.52, 2.85) mean 2.25
input std 100.0 -> output range (0.52, 2.85) mean 2.25

On z-scored nnU-Net patches that is finite, silent and badly out of distribution. It is the same failure the no-foreground branch of this very method carries a comment about ("turns a z-scored air patch … into [0, 1] … a perfectly finite, badly out-of-distribution patch that no NaN guard can see") — fixed there, not here.

What I tried, and why this PR is narrower than you might expect

The obvious fix is to undo the working normalisation before returning: x = x * denom + img_min. I implemented it and measured it, and it is worse. The perturbation is defined in normalised units, so rescaling it by the input range amplifies it:

before the "fix":  output mean  2.2
after  the "fix":  output mean 18.4

Bounding the amplitude in input units instead would be a redesign of an augmentation inherited from totalspineseg, not a bug fix, and I am not going to smuggle one into a bug-hunt PR.

So this PR does the two things that are defensible without redesigning it:

  1. retain_stats defaults to True — the default was the broken setting.
  2. The behaviour is written down in the class docstring, including the failed approach, so the next person does not repeat it.

Behaviour change

None for any shipped config. All six that use this transform set retain_stats explicitly. Both paper configs already use True; transform_params_hybrid.json and transform_params_hybrid_TAGE.json ask for False and keep it — they are still affected, and that is now documented rather than hidden.

all_augmentations.json is regenerated for the new default.

Tests

unit_tests/test_redistribute_scale.py: the default preserves the input's mean and std, still changes the image, and tracks the input scale across a 100× range. A separate class pins the known limitation — with retain_stats=False the output is identical for inputs differing only in scale — so it cannot be forgotten or silently change.

Full suite unchanged against baseline; ruff check . clean.

🤖 Generated with Claude Code

The method works in a per-sample [0, 1] min-max space and adds a perturbation of
up to 2.0 in that space -- twice the input's full dynamic range. With
`retain_stats=False` nothing maps the result back, so the output does not depend
on the input's scale at all: measured, inputs with std 1.0, 1.5 and 100.0 all
came out in [0.52, 2.85] with mean 2.25. On z-scored nnU-Net patches that is
finite, silent and badly out of distribution -- the same failure the
no-foreground branch of this method already carries a comment about.

Mapping back is not a fix on its own. The perturbation is defined in normalised
units, so rescaling it by the input range makes it much larger: I tried it and
the output mean went from 2.2 to 18.4. Bounding the amplitude in input units
would be a redesign of an augmentation inherited from totalspineseg, not a bug
fix, so this changes only which setting you get by default and writes the
limitation down.

Every shipped config sets retain_stats explicitly, so none of them moves. Both
paper configs already use True; transform_params_hybrid.json and
transform_params_hybrid_TAGE.json ask for False and keep it.

all_augmentations.json is regenerated for the new default.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant