Make NormalizeIntensity invertible (#5647)#8905
Conversation
Signed-off-by: azrabano23 <ab2895@scarletmail.rutgers.edu>
📝 WalkthroughWalkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
monai/transforms/intensity/dictionary.py (1)
835-839: ⚡ Quick winDocument
NormalizeIntensityd.inverse()with a Google-style docstring.The new
inversedefinition should describe input mapping, return mapping, and propagated exceptions fromself.normalizer.inverse.As per coding guidelines, “Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/transforms/intensity/dictionary.py` around lines 835 - 839, Add a Google-style docstring to the NormalizeIntensityd.inverse method that documents the input parameter `data` (Mapping[Hashable, NdarrayOrTensor]) and states that the method returns a dict[Hashable, NdarrayOrTensor] with normalized values, and explicitly documents that any exceptions raised by `self.normalizer.inverse` (propagated through the loop) may be raised by this method; include brief descriptions for Args, Returns, and Raises sections and reference `self.normalizer.inverse` in the Raises section so callers know which errors can surface.Source: Coding guidelines
monai/transforms/intensity/array.py (1)
966-985: ⚡ Quick winAdd Google-style docstrings for new methods.
_to_storable,_push_transform_with_stats, andinverseshould includeArgs/Returns/Raisessections to match the repo rule for new definitions.As per coding guidelines, “Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings.”
Also applies to: 986-1010
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/transforms/intensity/array.py` around lines 966 - 985, Add Google-style docstrings for the new methods _to_storable, _push_transform_with_stats, and inverse: for each function include an Args section describing each parameter (type and meaning), a Returns section describing the return value and its type, and a Raises section for any exceptions the method may raise (or state "None" if it does not raise). Reference the existing method names (_to_storable, _push_transform_with_stats, inverse) and ensure the docstrings follow the repo's Google-style format and cover torch.Tensor/np.ndarray handling, MetaTensor/out behavior, and any preconditions like get_track_meta() or expected types.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/transforms/intensity/array.py`:
- Around line 902-903: The code is storing None stats into invertible extra_info
(via _push_transform_with_stats) when _normalize() returns None for nonzero=True
with all-zero masks; update the logic so that when _normalize(...) returns None
you do not add any stats entries to extra_info (i.e., skip serializing or
setting keys) — modify callers (e.g., where _normalize is invoked in the
normalization transform) or update _push_transform_with_stats to check for None
and return early/omit adding the stats entry so extra_info never contains None
values.
In `@tests/transforms/test_normalize_intensity.py`:
- Around line 151-164: These tests call set_track_meta(True) but never restore
previous global state; wrap the body of each test (the ones creating MetaTensor
and using NormalizeIntensity/out/inv) by capturing the prior state with
get_track_meta() (or equivalent getter), call set_track_meta(True), then run the
test assertions in a try/finally and restore the original state with
set_track_meta(prev) in the finally block so global metadata tracking is always
returned to its prior value after test execution.
---
Nitpick comments:
In `@monai/transforms/intensity/array.py`:
- Around line 966-985: Add Google-style docstrings for the new methods
_to_storable, _push_transform_with_stats, and inverse: for each function include
an Args section describing each parameter (type and meaning), a Returns section
describing the return value and its type, and a Raises section for any
exceptions the method may raise (or state "None" if it does not raise).
Reference the existing method names (_to_storable, _push_transform_with_stats,
inverse) and ensure the docstrings follow the repo's Google-style format and
cover torch.Tensor/np.ndarray handling, MetaTensor/out behavior, and any
preconditions like get_track_meta() or expected types.
In `@monai/transforms/intensity/dictionary.py`:
- Around line 835-839: Add a Google-style docstring to the
NormalizeIntensityd.inverse method that documents the input parameter `data`
(Mapping[Hashable, NdarrayOrTensor]) and states that the method returns a
dict[Hashable, NdarrayOrTensor] with normalized values, and explicitly documents
that any exceptions raised by `self.normalizer.inverse` (propagated through the
loop) may be raised by this method; include brief descriptions for Args,
Returns, and Raises sections and reference `self.normalizer.inverse` in the
Raises section so callers know which errors can surface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6b1bc07-d035-40aa-863c-bf7b8c626aaa
📒 Files selected for processing (4)
monai/transforms/intensity/array.pymonai/transforms/intensity/dictionary.pytests/transforms/test_normalize_intensity.pytests/transforms/test_normalize_intensityd.py
| return img, None, None | ||
| else: |
There was a problem hiding this comment.
Avoid storing None in invertible extra_info.
For nonzero=True + all-zero mask, _normalize() returns None stats, then _push_transform_with_stats() serializes them into extra_info. That can break transform-history collation in batched pipelines.
Suggested fix
- if not slices.any():
- return img, None, None
+ if not slices.any():
+ # keep metadata collate-safe by storing identity normalization params
+ return img, 0.0, 1.0Also applies to: 977-980
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@monai/transforms/intensity/array.py` around lines 902 - 903, The code is
storing None stats into invertible extra_info (via _push_transform_with_stats)
when _normalize() returns None for nonzero=True with all-zero masks; update the
logic so that when _normalize(...) returns None you do not add any stats entries
to extra_info (i.e., skip serializing or setting keys) — modify callers (e.g.,
where _normalize is invoked in the normalization transform) or update
_push_transform_with_stats to check for None and return early/omit adding the
stats entry so extra_info never contains None values.
| set_track_meta(True) | ||
| img = MetaTensor(torch.randn(3, 6, 6) * 5 + 2) | ||
| normalizer = NormalizeIntensity(**args) | ||
| out = normalizer(img.clone()) | ||
| inv = normalizer.inverse(out) | ||
| assert_allclose(inv, img, type_test=False, rtol=1e-4, atol=1e-4) | ||
|
|
||
| def test_inverse_nonzero_not_implemented(self): | ||
| set_track_meta(True) | ||
| img = MetaTensor(torch.randn(2, 5, 5)) | ||
| normalizer = NormalizeIntensity(nonzero=True) | ||
| out = normalizer(img.clone()) | ||
| with self.assertRaises(NotImplementedError): | ||
| normalizer.inverse(out) |
There was a problem hiding this comment.
Restore track_meta global state after each test.
These tests set global metadata tracking to True and never restore prior state, which can make later tests order-dependent.
Suggested fix
-from monai.data import MetaTensor, set_track_meta
+from monai.data import MetaTensor, get_track_meta, set_track_meta
...
def test_inverse(self, _, args):
- set_track_meta(True)
+ prev = get_track_meta()
+ self.addCleanup(set_track_meta, prev)
+ set_track_meta(True)
...
def test_inverse_nonzero_not_implemented(self):
- set_track_meta(True)
+ prev = get_track_meta()
+ self.addCleanup(set_track_meta, prev)
+ set_track_meta(True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set_track_meta(True) | |
| img = MetaTensor(torch.randn(3, 6, 6) * 5 + 2) | |
| normalizer = NormalizeIntensity(**args) | |
| out = normalizer(img.clone()) | |
| inv = normalizer.inverse(out) | |
| assert_allclose(inv, img, type_test=False, rtol=1e-4, atol=1e-4) | |
| def test_inverse_nonzero_not_implemented(self): | |
| set_track_meta(True) | |
| img = MetaTensor(torch.randn(2, 5, 5)) | |
| normalizer = NormalizeIntensity(nonzero=True) | |
| out = normalizer(img.clone()) | |
| with self.assertRaises(NotImplementedError): | |
| normalizer.inverse(out) | |
| prev = get_track_meta() | |
| self.addCleanup(set_track_meta, prev) | |
| set_track_meta(True) | |
| img = MetaTensor(torch.randn(3, 6, 6) * 5 + 2) | |
| normalizer = NormalizeIntensity(**args) | |
| out = normalizer(img.clone()) | |
| inv = normalizer.inverse(out) | |
| assert_allclose(inv, img, type_test=False, rtol=1e-4, atol=1e-4) | |
| def test_inverse_nonzero_not_implemented(self): | |
| prev = get_track_meta() | |
| self.addCleanup(set_track_meta, prev) | |
| set_track_meta(True) | |
| img = MetaTensor(torch.randn(2, 5, 5)) | |
| normalizer = NormalizeIntensity(nonzero=True) | |
| out = normalizer(img.clone()) | |
| with self.assertRaises(NotImplementedError): | |
| normalizer.inverse(out) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/transforms/test_normalize_intensity.py` around lines 151 - 164, These
tests call set_track_meta(True) but never restore previous global state; wrap
the body of each test (the ones creating MetaTensor and using
NormalizeIntensity/out/inv) by capturing the prior state with get_track_meta()
(or equivalent getter), call set_track_meta(True), then run the test assertions
in a try/finally and restore the original state with set_track_meta(prev) in the
finally block so global metadata tracking is always returned to its prior value
after test execution.
|
What about the |
2 participants
Fixes #5647
Description
NormalizeIntensity(and its dict wrapperNormalizeIntensityd) is now invertible. As requested in #5647, the subtrahend and divisor actually used — whether passed in or computed from the image mean/std — are stored in the transform's meta information, so the normalization can be reversed (img * divisor + subtrahend). This is useful for reconstruction problems and for logging/recovering original intensities.Implementation follows the maintainer's suggestion in the issue thread (store the stats via
push_transform(..., extra_info=...)):NormalizeIntensitynow subclassesInvertibleTransform._normalizereturns thesub/divit used;__call__collects them (per channel whenchannel_wise=True, once otherwise) and pushes them onto the output MetaTensor's transform stack. A newinverse()reverses the operation.NormalizeIntensitydsubclassesInvertibleTransformand delegatesinverse()to the array transform per key.MetaTensorandget_track_meta()is True, so behaviour is unchanged when meta tracking is off.Scope note (
nonzero=True): inversion is intentionally not supported whennonzero=True, because reversing the masked normalization exactly would require storing the per-voxel zero mask. In that caseinverse()raises a clearNotImplementedErrorrather than returning an incorrect result. Happy to extend this (e.g. by optionally storing the mask) if maintainers prefer.Types of changes
./runtests.sh -f -u --net --coverage. (ran the relevant unit tests + formatting/lint locally — see below)./runtests.sh --quick --unittests --disttests. (ran the affected suites)Testing
New tests assert round-trip
inverse(transform(x)) ≈ xfor global and channel-wise modes with both computed and explicitly-provided sub/div, and thatnonzero=TrueraisesNotImplementedErroron inverse.isort,black, andruff checkall pass on the changed files.