feat: Add RandNonCentralChiNoise transform #8618
Conversation
karllandheer
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces RandNonCentralChiNoise (array RandomizableTransform) and a dictionary wrapper RandNonCentralChiNoised with aliases RandNonCentralChiNoiseD / RandNonCentralChiNoiseDict; adds unit tests for both array and dict variants; updates monai/transforms/init.py and CHANGELOG.md to export the new symbols; and appends monai-dev/ to .gitignore. The array transform implements non-central chi sampling with degrees_of_freedom validation, channel-wise vs global modes, relative/sample_std options, and dtype handling; the dict wrapper preserves MapTransform semantics and forwards RNG state. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas needing attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds RandNonCentralChiNoise and RandNonCentralChiNoised transforms to generalize Rician noise simulation for modern MRI systems with multiple quadrature coils.
Key Changes:
- Implemented
RandNonCentralChiNoiseinarray.pywith configurable degrees of freedom (k), defaulting to 64 for typical 32-channel head coils - Added dictionary-based wrapper
RandNonCentralChiNoisedfollowing MONAI patterns - Comprehensive test coverage for both k=64 and k=2 (Rician) cases with numerical validation
- Updated exports in
__init__.pyand CHANGELOG - Mathematical implementation: computes sqrt(sum(k Gaussian noise arrays²)) where first array includes the signal
Implementation Quality:
- Follows existing
RandRicianNoisepatterns closely for consistency - Supports both NumPy and PyTorch backends
- Proper validation for degrees_of_freedom parameter (must be int ≥ 1)
- Channel-wise and relative noise options maintained
- Tests verify correctness against manual calculations
Confidence Score: 5/5
- Safe to merge - well-tested feature addition with proper validation and comprehensive test coverage
- Clean implementation following established patterns, comprehensive tests covering edge cases (k=2 Rician case and k=64), proper input validation, and no breaking changes to existing code
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| monai/transforms/intensity/array.py | 4/5 | Added RandNonCentralChiNoise transform generalizing Rician noise to k degrees of freedom with proper validation and both NumPy/Torch support |
| monai/transforms/intensity/dictionary.py | 5/5 | Added RandNonCentralChiNoised dictionary wrapper with proper random state management and key iteration |
| tests/transforms/test_rand_noncentralchi_noise.py | 5/5 | Comprehensive tests for array transform covering zero/non-zero mean, k=64 and k=2 (Rician) cases with numerical validation |
| tests/transforms/test_rand_noncentralchi_noised.py | 5/5 | Comprehensive tests for dictionary transform covering multiple keys, k=64 and k=2 cases with numerical validation |
Sequence Diagram
sequenceDiagram
participant User
participant RandNonCentralChiNoise
participant RandomizableTransform
participant _add_noise
participant NumPy/Torch
User->>RandNonCentralChiNoise: __call__(img, randomize=True)
RandNonCentralChiNoise->>RandNonCentralChiNoise: convert_to_tensor(img, dtype)
RandNonCentralChiNoise->>RandomizableTransform: randomize(None)
RandomizableTransform-->>RandNonCentralChiNoise: set _do_transform based on prob
alt not _do_transform
RandNonCentralChiNoise-->>User: return original img
else _do_transform
alt channel_wise
loop for each channel
RandNonCentralChiNoise->>_add_noise: _add_noise(channel, mean, std, k)
_add_noise->>NumPy/Torch: sample k Gaussian noise arrays
_add_noise->>_add_noise: add img to first noise array
_add_noise->>NumPy/Torch: compute sqrt(sum(noises²))
_add_noise-->>RandNonCentralChiNoise: noised channel
end
else not channel_wise
RandNonCentralChiNoise->>_add_noise: _add_noise(img, mean, std, k)
_add_noise->>NumPy/Torch: sample k Gaussian noise arrays (shape: k×img.shape)
_add_noise->>_add_noise: add img to first noise array [0]
_add_noise->>NumPy/Torch: compute sqrt(sum(noises²)) along axis 0
_add_noise-->>RandNonCentralChiNoise: noised img
end
RandNonCentralChiNoise-->>User: return noised img
end
5 files reviewed, 2 comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/transforms/intensity/dictionary.py (1)
70-174: Missing public exports for alias variants.The aliases
RandNonCentralChiNoiseDandRandNonCentralChiNoiseDict(defined at line 2032) must be added to__all__to match the pattern of other transforms.Add these entries after line 73:
"RandGaussianNoised", "RandRicianNoised", "RandNonCentralChiNoised", + "RandNonCentralChiNoiseD", + "RandNonCentralChiNoiseDict", "ShiftIntensityd",
🧹 Nitpick comments (2)
monai/transforms/intensity/array.py (1)
176-176: Missing space after comment hash.Line 176 comment should be
# 64 default because...not#64 default because...Apply this diff:
- degrees_of_freedom: int = 64, #64 default because typical modern brain MRI is 32 quadrature coils + degrees_of_freedom: int = 64, # 64 default because typical modern brain MRI is 32 quadrature coilstests/transforms/test_rand_noncentralchi_noise.py (1)
33-33: Missing space after comment hash.Line 33 should be
# 64 is common...not#64 is common...Apply this diff:
- degrees_of_freedom = 64 #64 is common due to 32 channel head coil + degrees_of_freedom = 64 # 64 is common due to 32 channel head coil
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)monai/transforms/__init__.py(2 hunks)monai/transforms/intensity/array.py(2 hunks)monai/transforms/intensity/dictionary.py(4 hunks)tests/transforms/test_rand_noncentralchi_noise.py(1 hunks)tests/transforms/test_rand_noncentralchi_noised.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit configuration file
Remember that documentation must be updated with the latest information.
Files:
CHANGELOG.md
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/transforms/test_rand_noncentralchi_noise.pymonai/transforms/intensity/array.pymonai/transforms/intensity/dictionary.pytests/transforms/test_rand_noncentralchi_noised.pymonai/transforms/__init__.py
🪛 Ruff (0.14.2)
monai/transforms/intensity/array.py
169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
187-187: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
242-242: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (windows-latest)
🔇 Additional comments (7)
monai/transforms/intensity/array.py (2)
194-212: LGTM!The
_add_noiseimplementation correctly follows the non-central chi distribution: generates k Gaussian noise components, adds the signal to the first component, then computes the magnitude. Handles both torch and numpy backends.
214-244: LGTM!The
__call__method correctly handles both channel-wise and global noise application. Type validation for mean/std parameters prevents runtime errors. Implementation is consistent withRandRicianNoise.tests/transforms/test_rand_noncentralchi_noise.py (1)
29-79: LGTM!Tests comprehensively verify correctness by replaying the random generation process and comparing against analytically computed expected results. Coverage includes both k=64 (typical MRI) and k=2 (Rician noise special case), with both zero and non-zero mean scenarios.
tests/transforms/test_rand_noncentralchi_noised.py (2)
35-35: Verify dtype choice.Tests use
dtype=np.float64while the array-based tests use default dtype (float32). Is higher precision intentionally required for dictionary variant testing, or should this match the array tests?
31-73: LGTM!Tests verify dictionary-based transform correctness by replaying random generation per key and comparing against expected results. Coverage includes multiple keys, both k=64 and k=2, and various mean/std combinations.
monai/transforms/intensity/dictionary.py (2)
241-313: LGTM - Implementation follows correct pattern.The implementation correctly follows the
RandRicianNoisedpattern with independent noise per key (line 312), which is appropriate since non-central chi noise generalizes Rician noise. Docstring is comprehensive, type hints are correct, and random state propagation is properly implemented.
2032-2032: Aliases correctly defined.Follows MONAI naming convention pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
monai/transforms/intensity/array.py (1)
171-192: Remove redundant assignment.Line 183 assigns
self.prob = prob, butRandomizableTransform.__init__(self, prob)on line 182 already sets this attribute.def __init__( self, prob: float = 0.1, mean: Sequence[float] | float = 0.0, std: Sequence[float] | float = 1.0, degrees_of_freedom: int = 64, # 64 default because typical modern brain MRI is 32 quadrature coils channel_wise: bool = False, relative: bool = False, sample_std: bool = True, dtype: DtypeLike = np.float32, ) -> None: RandomizableTransform.__init__(self, prob) - self.prob = prob self.mean = mean self.std = std if not isinstance(degrees_of_freedom, int) or degrees_of_freedom < 1: raise ValueError("degrees_of_freedom must be an integer >= 1.") self.degrees_of_freedom = degrees_of_freedom self.channel_wise = channel_wise self.relative = relative self.sample_std = sample_std self.dtype = dtypetests/transforms/test_rand_noncentralchi_noise.py (1)
29-79: Consider testing additional modes.Current tests validate core correctness well. For more comprehensive coverage, consider adding tests for
channel_wise=Trueandrelative=Truemodes, which have distinct code paths in the implementation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/intensity/array.py(2 hunks)tests/transforms/test_rand_noncentralchi_noise.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/transforms/test_rand_noncentralchi_noise.pymonai/transforms/intensity/array.py
🪛 Ruff (0.14.2)
monai/transforms/intensity/array.py
169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
187-187: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
242-242: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
monai/transforms/intensity/array.py (3)
42-44: LGTM - proper module export.The new transform is correctly added to
__all__with appropriate alphabetical positioning.
194-212: LGTM - correct non-central chi noise implementation.The method properly implements non-central chi distribution by adding the image to the first of k Gaussian noise channels, then computing the root sum of squares. Handles both Torch and NumPy paths efficiently.
214-244: LGTM - robust implementation with proper validation.The
__call__method correctly handles both channel-wise and global noise application modes, with appropriate type checking and relative scaling support. The pattern is consistent with similar transforms in the codebase.tests/transforms/test_rand_noncentralchi_noise.py (2)
29-52: LGTM - thorough correctness validation.Test properly validates the transform by manually replicating the noise generation with controlled seeding. Checks dtype preservation and numerical correctness with appropriate tolerance.
54-79: LGTM - validates Rician noise special case.Explicitly testing degrees_of_freedom=2 is valuable since Rician noise is a well-known special case of non-central chi distribution mentioned in the transform documentation.
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds RandNonCentralChiNoise and RandNonCentralChiNoised transforms to generalize Rician noise (k=2) to arbitrary degrees of freedom (k), enabling accurate noise simulation for modern MRI with 32+ quadrature coils.
Key changes:
- Implements array transform
RandNonCentralChiNoisewith proper input validation (degrees_of_freedommust be int >= 1) - Adds dictionary wrapper
RandNonCentralChiNoisedfollowing MONAI's established patterns - Supports both PyTorch and NumPy backends with proper device handling
- Includes comprehensive tests verifying mathematical correctness for k=2 (Rician case) and k=64
- Properly exports new transforms in
__init__.pyand updates CHANGELOG - Follows existing codebase patterns from
RandRicianNoisefor consistency
The implementation correctly models the non-central chi distribution by generating k Gaussian noise arrays, adding the signal to the first array, and computing the magnitude as sqrt(sum(squares)). The code structure, error handling, and testing approach align well with MONAI conventions.
Confidence Score: 4/5
- This PR is safe to merge with minor considerations
- The implementation follows established MONAI patterns closely (mirroring
RandRicianNoise), includes proper input validation, comprehensive tests with mathematical verification, and supports both major backends. Score of 4 rather than 5 due to limited test coverage of edge cases (channel_wise, relative, sample_std=False modes not explicitly tested) and the addition of a personal dev directory to .gitignore. - No files require special attention - all implementations follow established patterns and include proper validation
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| monai/transforms/intensity/array.py | 4/5 | Added RandNonCentralChiNoise transform that generalizes Rician noise to k degrees of freedom, with proper validation and support for both PyTorch and NumPy backends |
| monai/transforms/intensity/dictionary.py | 5/5 | Added dictionary wrapper RandNonCentralChiNoised following existing MONAI patterns for dictionary transforms |
| tests/transforms/test_rand_noncentralchi_noise.py | 4/5 | Comprehensive tests for array transform with zero/non-zero mean cases and k=2 (Rician) validation |
| tests/transforms/test_rand_noncentralchi_noised.py | 4/5 | Dictionary transform tests covering multiple keys with proper randomization state verification |
| .gitignore | 5/5 | Added monai-dev/ directory to gitignore (developer's local directory) |
Sequence Diagram
sequenceDiagram
participant User
participant RandNonCentralChiNoised
participant RandNonCentralChiNoise
participant RandomState
User->>RandNonCentralChiNoised: __call__(data_dict)
RandNonCentralChiNoised->>RandomState: randomize() - check prob
alt transform should apply
loop for each key in data_dict
RandNonCentralChiNoised->>RandNonCentralChiNoise: __call__(img, randomize=True)
RandNonCentralChiNoise->>RandomState: randomize() - check prob
RandNonCentralChiNoise->>RandNonCentralChiNoise: _add_noise(img, mean, std, k)
RandNonCentralChiNoise->>RandomState: uniform(0, std) if sample_std
RandNonCentralChiNoise->>RandomState: normal(mean, std, size=(k, ...))
Note over RandNonCentralChiNoise: Generate k Gaussian noise arrays
Note over RandNonCentralChiNoise: Add signal to first array: noises[0] += img
Note over RandNonCentralChiNoise: Compute sqrt(sum(noises^2))
RandNonCentralChiNoise-->>RandNonCentralChiNoised: noised_img
end
RandNonCentralChiNoised-->>User: noised_data_dict
else skip transform
RandNonCentralChiNoised-->>User: original_data_dict
end
5 files reviewed, no comments
Adds RandNonCentralChiNoise and RandNonCentralChiNoised, which generalize Rician noise to k degrees of freedom. Standard brain MRI typically uses 32 (or more) quadrature coils, so accurate noise simulation requires this modification, especially in the low SNR limit Includes array, dictionary, and test files. Signed-off-by: Karl Landheer <[email protected]> Signed-off-by: karllandheer <[email protected]>
karllandheer
force-pushed
the
feature/add-non-central-chi-noise
branch
from
20e9189 to
82c0fb3
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)monai/transforms/__init__.py(2 hunks)monai/transforms/intensity/array.py(2 hunks)monai/transforms/intensity/dictionary.py(4 hunks)tests/transforms/test_rand_noncentralchi_noise.py(1 hunks)tests/transforms/test_rand_noncentralchi_noised.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- CHANGELOG.md
- tests/transforms/test_rand_noncentralchi_noise.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/__init__.pytests/transforms/test_rand_noncentralchi_noised.pymonai/transforms/intensity/dictionary.pymonai/transforms/intensity/array.py
🪛 Ruff (0.14.3)
monai/transforms/intensity/array.py
169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
187-187: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
242-242: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
Fixing the checks from the CI pipeline. Signed-off-by: karllandheer <[email protected]>
Fixes metadata-dropping bug by implementing the src/convert_to_dst_type round-trip. Fixes isort error by re-ordering imports in transforms/__init__.py. Signed-off-by: karllandheer <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/transforms/intensity/array.py (1)
275-282: Consider validating mean/std types in__init__whenchannel_wise=False.Type checks occur on every
__call__invocation. Moving validation to initialization provides earlier feedback.def __init__(self, ...): ... if not channel_wise: if not isinstance(mean, (int, float)): raise ValueError(f"If channel_wise is False, mean must be a float or int, got {type(mean)}.") if not isinstance(std, (int, float)): raise ValueError(f"If channel_wise is False, std must be a float or int, got {type(std)}.")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
monai/transforms/__init__.py(2 hunks)monai/transforms/intensity/array.py(2 hunks)monai/transforms/intensity/dictionary.py(4 hunks)tests/transforms/test_rand_noncentralchi_noised.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/transforms/init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/intensity/dictionary.pymonai/transforms/intensity/array.pytests/transforms/test_rand_noncentralchi_noised.py
🪛 Ruff (0.14.3)
monai/transforms/intensity/array.py
286-286: Expected a statement
(invalid-syntax)
286-286: Expected a statement
(invalid-syntax)
286-286: Expected a statement
(invalid-syntax)
286-286: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-291: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
294-294: Expected ',', found name
(invalid-syntax)
294-294: Expected ',', found name
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
monai/transforms/intensity/array.py (1)
286-294: CRITICAL: Unresolved merge conflict.Git conflict markers present in the code. Remove markers and reconcile changes before merging.
-<<<<<<< HEAD - - - -======= - - - ->>>>>>> 83d8ad52 (Fixes metadata dropping) + + +
🧹 Nitpick comments (1)
monai/transforms/intensity/array.py (1)
176-176: Style: Missing space after#.Inline comment should have a space after
#.- degrees_of_freedom: int = 64, # 64 default because typical modern brain MRI is 32 quadrature coils + degrees_of_freedom: int = 64, # 64 default because typical modern brain MRI is 32 quadrature coilsBased on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/transforms/intensity/array.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/intensity/array.py
🪛 Ruff (0.14.3)
monai/transforms/intensity/array.py
286-286: Expected a statement
(invalid-syntax)
286-286: Expected a statement
(invalid-syntax)
286-286: Expected a statement
(invalid-syntax)
286-286: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-290: Expected a statement
(invalid-syntax)
290-291: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Expected a statement
(invalid-syntax)
294-294: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
294-294: Expected ',', found name
(invalid-syntax)
294-294: Expected ',', found name
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: packaging
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
🔇 Additional comments (5)
monai/transforms/intensity/array.py (5)
44-44: LGTM on export.Correct alphabetical placement in
__all__.
206-208: Proper validation.
degrees_of_freedomvalidation correctly enforces integer >= 1.
214-249: Noise generation implementation is sound.The
_add_noisemethod correctly implements non-central chi distribution: generates k Gaussian noise arrays, adds signal to first component, computes sqrt of sum of squares. Proper backend handling for torch and numpy.
255-262: Metadata preservation is correct.The transform properly preserves input metadata via
convert_to_dst_typecalls, ensuring MetaTensor affine/meta survives the transform.Also applies to: 284-285
264-283: Channel-wise and global modes correctly implemented.Logic correctly handles both channel-wise processing (with per-channel mean/std) and global processing (with type validation for scalar mean/std). Consistent with
RandRicianNoisepattern.
Fixes metadata-dropping bug by implementing the src/convert_to_dst_type round-trip. Fixes isort error by re-ordering imports in transforms/__init__.py. Signed-off-by: karllandheer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/transforms/intensity/array.py (1)
176-176: PEP8: Add space after#in inline comment.- degrees_of_freedom: int = 64, # 64 default because typical modern brain MRI is 32 quadrature coils + degrees_of_freedom: int = 64, # 64 default because typical modern brain MRI is 32 quadrature coils
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/transforms/intensity/array.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/intensity/array.py
🪛 Ruff (0.14.3)
monai/transforms/intensity/array.py
169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
207-207: Avoid specifying long messages outside the exception class
(TRY003)
276-276: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
🔇 Additional comments (3)
monai/transforms/intensity/array.py (3)
44-44: LGTM: Export properly positioned.
214-249: LGTM: Noise generation logic is mathematically sound.The non-central chi distribution implementation correctly:
- Creates k Gaussian noise components
- Adds the signal to the first component (non-centrality)
- Computes magnitude via sqrt(sum of squares)
- Handles both torch and numpy backends
251-285: LGTM: Metadata preservation correctly implemented.The transform properly:
- Saves the original input reference before conversion
- Handles randomization with early return
- Applies channel-wise or global noise modes
- Validates types when channel_wise=False
- Converts back to original type/metadata via convert_to_dst_type
This addresses the metadata dropping issue from previous reviews.
Signed-off-by: karllandheer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
monai/transforms/intensity/array.py(15 hunks)monai/transforms/intensity/dictionary.py(16 hunks)tests/transforms/test_rand_noncentralchi_noise.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/transforms/test_rand_noncentralchi_noise.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/intensity/array.pymonai/transforms/intensity/dictionary.py
🪛 Ruff (0.14.3)
monai/transforms/intensity/array.py
169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
207-207: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Avoid specifying long messages outside the exception class
(TRY003)
1404-1404: Avoid specifying long messages outside the exception class
(TRY003)
2222-2222: Prefer TypeError exception for invalid type
(TRY004)
2222-2222: Avoid specifying long messages outside the exception class
(TRY003)
2831-2831: Avoid specifying long messages outside the exception class
(TRY003)
2842-2842: Prefer TypeError exception for invalid type
(TRY004)
2842-2842: Avoid specifying long messages outside the exception class
(TRY003)
2964-2964: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
monai/transforms/intensity/array.py (4)
144-169: LGTM - Clean class structure.The class definition, docstring, and backend declaration follow MONAI conventions. The docstring clearly explains non-central chi noise as a generalization of Rician noise.
171-212: LGTM - Proper parameter validation.The initialization correctly validates
degrees_of_freedomas an integer >= 1 and stores all parameters. Default of 64 aligns with 32 quadrature coils mentioned in the comment.
214-248: LGTM - Correct non-central chi noise implementation.The noise generation correctly:
- Creates k independent Gaussian noise arrays
- Adds the input image to the first array (non-centrality component)
- Computes sqrt(sum of squares)
- Handles both NumPy and PyTorch backends
250-284: LGTM - Proper metadata preservation.The
__call__method correctly preserves metadata viaconvert_to_dst_typeat lines 260 and 283, addressing past review feedback. Channel-wise and global modes both handle dtype conversion properly.monai/transforms/intensity/dictionary.py (2)
241-299: LGTM - Well-structured dictionary wrapper.The class follows MapTransform conventions with proper initialization, random state propagation, and comprehensive docstring.
2005-2005: LGTM - Proper aliasing for API consistency.Aliases follow the established pattern (D suffix, Dict suffix) used by other transforms in the module.
| def __call__(self, data: Mapping[Hashable, NdarrayOrTensor]) -> dict[Hashable, NdarrayOrTensor]: | ||
| d = dict(data) | ||
| self.randomize(None) | ||
| if not self._do_transform: | ||
| for key in self.key_iterator(d): | ||
| d[key] = convert_to_tensor(d[key], track_meta=get_track_meta()) | ||
| return d | ||
|
|
||
| for key in self.key_iterator(d): | ||
| d[key] = self.rand_non_central_chi_noise(d[key], randomize=True) | ||
| return d |
There was a problem hiding this comment.
Critical: Fix randomize=True to prevent inconsistent per-key behavior.
Line 310 uses randomize=True, causing _do_transform to be re-randomized for each key. This means some keys may be transformed while others are not, which contradicts the transform/no-transform decision made at line 303.
Apply this fix:
for key in self.key_iterator(d):
- d[key] = self.rand_non_central_chi_noise(d[key], randomize=True)
+ d[key] = self.rand_non_central_chi_noise(d[key], randomize=False)
return dNote: RandRicianNoised at line 378 has the same bug and should be fixed separately.
🤖 Prompt for AI Agents
monai/transforms/intensity/dictionary.py lines 301-311: the call
rand_non_central_chi_noise(..., randomize=True) re-randomizes the transform
decision per key causing inconsistent per-key behavior; change it to call
rand_non_central_chi_noise(..., randomize=False) (or omit the randomize argument
if False is the default) so the per-call randomize set by self.randomize(None)
at line 303 governs all keys consistently; also apply the same fix to the
analogous call in RandRicianNoised around line 378.
Signed-off-by: karllandheer <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
monai/transforms/intensity/dictionary.py (1)
305-315: Stop re-randomizing per key.Calling the array transform with
randomize=Trueinside the loop flips_do_transformanew for every key, so some dictionary entries get no noise even though the wrapper already decided to apply it. Randomize once up front and forwardrandomize=Falseto honour the “all-or-nothing” contract.if not self._do_transform: for key in self.key_iterator(d): d[key] = convert_to_tensor(d[key], track_meta=get_track_meta()) return d + first_key: Hashable = self.first_key(d) + if first_key == (): + for key in self.key_iterator(d): + d[key] = convert_to_tensor(d[key], track_meta=get_track_meta()) + return d + self.rand_non_central_chi_noise.randomize(d[first_key]) + for key in self.key_iterator(d): - d[key] = self.rand_non_central_chi_noise(d[key], randomize=True) + d[key] = self.rand_non_central_chi_noise(d[key], randomize=False) return d
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/intensity/array.py(2 hunks)monai/transforms/intensity/dictionary.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/intensity/dictionary.pymonai/transforms/intensity/array.py
🪛 Ruff (0.14.3)
monai/transforms/intensity/array.py
169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
207-207: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: quick-py3 (ubuntu-latest)
| img[i] = self._add_noise( | ||
| d, | ||
| mean=_mean[i], | ||
| std=_std[i] * d.std() if self.relative else _std[i], | ||
| k=self.degrees_of_freedom, | ||
| ) |
There was a problem hiding this comment.
Fix CUDA crash for relative channel noise.
Passing d.std() straight into np.random.uniform works on CPU but throws TypeError: can't convert CUDA tensor to numpy. Use Tensor.cpu() to copy the tensor to host memory first. when the input lives on GPU (the common case for MONAI). Convert the statistic to a host-side Python float before handing it to the RNG.
- for i, d in enumerate(img):
- img[i] = self._add_noise(
- d,
- mean=_mean[i],
- std=_std[i] * d.std() if self.relative else _std[i],
- k=self.degrees_of_freedom,
- )
+ for i, d in enumerate(img):
+ if self.relative:
+ channel_std = (
+ d.detach().std().cpu().item() if isinstance(d, torch.Tensor) else float(np.asarray(d).std())
+ )
+ std_arg = _std[i] * channel_std
+ else:
+ std_arg = _std[i]
+ img[i] = self._add_noise(
+ d,
+ mean=_mean[i],
+ std=std_arg,
+ k=self.degrees_of_freedom,
+ )🤖 Prompt for AI Agents
In monai/transforms/intensity/array.py around lines 267 to 272, the call that
passes d.std() into the RNG can crash on CUDA tensors; replace passing the
tensor directly with a host-side Python float by computing the
standard-deviation as a CPU scalar (e.g., use tensor.detach().cpu().item() or
float(...) for numpy arrays) and pass that float into the RNG so the RNG always
receives a Python number rather than a CUDA tensor.
1 participant
Adds RandNonCentralChiNoise and RandNonCentralChiNoised, which generalize Rician noise to k degrees of freedom. Standard brain MRI typically uses 32 (or more) quadrature coils, so accurate noise simulation requires this modification, especially in the low SNR limit.
Includes array, dictionary, and test files.
Fixes # .
Description
A few sentences describing the changes proposed in this pull request.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.