This PR adds clinical DICOM preprocessing pipelines for CT and MRI modalities, along with unit tests. #8659
Conversation
…nd example notebooks
for more information, see https://pre-commit.ci
Hitendrasinhdata7
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
WalkthroughAdds monai/transforms/clinical_preprocessing.py introducing ModalityTypeError and UnsupportedModalityError and three public functions: get_ct_preprocessing_pipeline (Compose of LoadImage(image_only=True), EnsureChannelFirst, ScaleIntensityRange(-1000, 400, 0, 1, clip=True)), get_mri_preprocessing_pipeline (Compose of LoadImage(image_only=True), EnsureChannelFirst, NormalizeIntensity(nonzero=True)), and preprocess_dicom_series (validates modality type, normalizes/aliases modality values, selects CT or MR pipeline, applies it, and raises type/unsupported errors). Adds tests in monai/tests/test_clinical_preprocessing.py covering pipeline composition, error messages for invalid modality/type, and preprocess behavior for CT/MR including case and whitespace variants with mocked pipelines. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/transforms/clinical_preprocessing.py (1)
14-14: Consider using a tuple for SUPPORTED_MODALITIES.The current string constant works for error messages, but a tuple would better represent a collection of discrete values and allow programmatic iteration.
-SUPPORTED_MODALITIES = "CT, MR, MRI" +SUPPORTED_MODALITIES = ("CT", "MR", "MRI")Then update line 89 to format it:
- raise ValueError(f"Unsupported modality: {modality}. Supported values: {SUPPORTED_MODALITIES}") + raise ValueError(f"Unsupported modality: {modality}. Supported values: {', '.join(SUPPORTED_MODALITIES)}")
📜 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 ignored due to path filters (2)
docs/clinical_dicom_workflow.pdfis excluded by!**/*.pdfmonai/docs/clinical_dicom_workflow.pdfis excluded by!**/*.pdf
📒 Files selected for processing (2)
monai/tests/test_clinical_preprocessing.py(1 hunks)monai/transforms/clinical_preprocessing.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:
monai/transforms/clinical_preprocessing.pymonai/tests/test_clinical_preprocessing.py
🧬 Code graph analysis (2)
monai/transforms/clinical_preprocessing.py (4)
monai/transforms/compose.py (1)
Compose(123-393)monai/transforms/io/array.py (1)
LoadImage(109-305)monai/transforms/utility/array.py (1)
EnsureChannelFirst(175-234)monai/transforms/intensity/array.py (2)
ScaleIntensityRange(959-1013)NormalizeIntensity(816-925)
monai/tests/test_clinical_preprocessing.py (2)
monai/transforms/intensity/array.py (2)
ScaleIntensityRange(959-1013)NormalizeIntensity(816-925)monai/transforms/clinical_preprocessing.py (3)
get_ct_preprocessing_pipeline(17-39)get_mri_preprocessing_pipeline(42-58)preprocess_dicom_series(61-91)
🪛 Ruff (0.14.8)
monai/transforms/clinical_preprocessing.py
64-64: Undefined name MetaTensor
(F821)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
monai/tests/test_clinical_preprocessing.py
41-41: Unused function argument: mock_loadimage
(ARG001)
51-51: Unused function argument: mock_loadimage
(ARG001)
⏰ 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-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: build-docs
- GitHub Check: packaging
🔇 Additional comments (11)
monai/tests/test_clinical_preprocessing.py (8)
1-13: LGTM!Imports and module structure are appropriate for testing clinical preprocessing utilities.
15-26: LGTM!Direct transform test correctly validates CT windowing behavior with appropriate tolerances.
28-38: LGTM!Direct transform test correctly validates MRI normalization with appropriate statistical tolerances.
40-48: LGTM!Pipeline construction test correctly validates the transform sequence. The
mock_loadimageparameter flagged by static analysis is required by the@patchdecorator.
50-58: LGTM!Pipeline construction test correctly validates the MRI transform sequence.
60-98: LGTM!Modality dispatch tests comprehensively validate CT and MRI pipeline selection, including case-insensitive handling and modality aliases.
100-120: LGTM!Error handling tests thoroughly validate TypeError and ValueError cases with appropriate message checks.
122-129: LGTM!Whitespace handling test validates input normalization for robust modality parsing.
monai/transforms/clinical_preprocessing.py (3)
17-39: LGTM!CT preprocessing pipeline correctly applies standard HU windowing [-1000, 400] mapped to [0.0, 1.0]. Docstring is clear and complete.
42-58: LGTM!MRI preprocessing pipeline correctly applies intensity normalization with
nonzero=Trueto handle zero-valued background. Docstring is clear and complete.
61-91: LGTM!Dispatcher function correctly validates input type, normalizes modality string, and selects appropriate pipeline. Error messages are informative. Docstring is complete with all sections.
…ew: add MetaTensor type hint, tuple for SUPPORTED_MODALITIES, and improved error messages
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
monai/transforms/clinical_preprocessing.py (1)
81-93: Consider extracting error messages to constants or custom exceptions.Static analysis flags the inline error messages. While the current approach is functional, extracting them improves maintainability.
+class UnsupportedModalityError(ValueError): + """Raised when an unsupported modality is provided.""" + pass + +class ModalityTypeError(TypeError): + """Raised when modality is not a string.""" + pass + def preprocess_dicom_series( dicom_path: Union[str, bytes, PathLike], modality: str, ) -> MetaTensor: # ... docstring ... if not isinstance(modality, str): - raise TypeError(f"modality must be a string, got {type(modality).__name__}") + raise ModalityTypeError(f"modality must be a string, got {type(modality).__name__}") modality = modality.strip().upper() if modality == "CT": transform = get_ct_preprocessing_pipeline() elif modality in ("MR", "MRI"): transform = get_mri_preprocessing_pipeline() else: - raise ValueError( - f"Unsupported modality: {modality}. Supported values: {', '.join(SUPPORTED_MODALITIES)}" - ) + raise UnsupportedModalityError( + f"Unsupported modality: {modality}. Supported values: {', '.join(SUPPORTED_MODALITIES)}" + )monai/tests/test_clinical_preprocessing.py (1)
56-75: Use isinstance checks instead of string comparison.Comparing class names with strings is brittle if classes are renamed or aliased.
Apply this diff for both pipeline tests:
def test_ct_preprocessing_pipeline(): """Test CT preprocessing pipeline returns expected transform composition.""" + from monai.transforms import LoadImage, EnsureChannelFirst, ScaleIntensityRange pipeline = get_ct_preprocessing_pipeline() assert hasattr(pipeline, 'transforms') assert len(pipeline.transforms) == 3 - assert pipeline.transforms[0].__class__.__name__ == 'LoadImage' - assert pipeline.transforms[1].__class__.__name__ == 'EnsureChannelFirst' - assert pipeline.transforms[2].__class__.__name__ == 'ScaleIntensityRange' + assert isinstance(pipeline.transforms[0], LoadImage) + assert isinstance(pipeline.transforms[1], EnsureChannelFirst) + assert isinstance(pipeline.transforms[2], ScaleIntensityRange) def test_mri_preprocessing_pipeline(): """Test MRI preprocessing pipeline returns expected transform composition.""" + from monai.transforms import LoadImage, EnsureChannelFirst, NormalizeIntensity pipeline = get_mri_preprocessing_pipeline() assert hasattr(pipeline, 'transforms') assert len(pipeline.transforms) == 3 - assert pipeline.transforms[0].__class__.__name__ == 'LoadImage' - assert pipeline.transforms[1].__class__.__name__ == 'EnsureChannelFirst' - assert pipeline.transforms[2].__class__.__name__ == 'NormalizeIntensity' + assert isinstance(pipeline.transforms[0], LoadImage) + assert isinstance(pipeline.transforms[1], EnsureChannelFirst) + assert isinstance(pipeline.transforms[2], NormalizeIntensity)
📜 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/tests/test_clinical_preprocessing.py(1 hunks)monai/transforms/clinical_preprocessing.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:
monai/tests/test_clinical_preprocessing.pymonai/transforms/clinical_preprocessing.py
🧬 Code graph analysis (1)
monai/tests/test_clinical_preprocessing.py (1)
monai/transforms/clinical_preprocessing.py (3)
get_ct_preprocessing_pipeline(19-41)get_mri_preprocessing_pipeline(44-60)preprocess_dicom_series(63-95)
🪛 Ruff (0.14.8)
monai/tests/test_clinical_preprocessing.py
130-130: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
142-142: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
144-144: Local variable error_message is assigned to but never used
Remove assignment to unused variable error_message
(F841)
monai/transforms/clinical_preprocessing.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
91-93: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
monai/transforms/clinical_preprocessing.py (3)
1-16: LGTM!Module structure is clean. MetaTensor is properly imported, addressing the previous review comment. Using a tuple for
SUPPORTED_MODALITIESis appropriate.
19-41: CT preprocessing pipeline is well-configured.The HU window [-1000, 400] is standard for soft tissue imaging. Including
clip=Trueprevents out-of-range values. Docstring is complete and follows Google style.
44-60: MRI preprocessing pipeline is appropriate.Using
NormalizeIntensity(nonzero=True)correctly handles background voxels in MRI. Docstring is complete and follows Google style.monai/tests/test_clinical_preprocessing.py (3)
12-34: Direct transform test is thorough.Testing ScaleIntensityRange with synthetic data validates the windowing logic. Shape preservation, finiteness, and range checks with tolerance are appropriate.
37-53: Direct normalization test is sound.Testing NormalizeIntensity validates the statistical properties. Tolerance values (atol=0.1) are reasonable for normalized data.
78-124: Dispatcher tests with mocks are well-structured.Good coverage of case-insensitive modality handling (CT, ct, MRI, MR). Mocks appropriately isolate the dispatcher logic.
…e checks in tests, and improve error handling
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
monai/tests/test_clinical_preprocessing.py (2)
12-19: Consider verifying transform parameters.Test checks composition but not ScaleIntensityRange parameters. Add assertions for
a_min,a_max,b_min,b_max,clipto catch parameter regressions.def test_ct_preprocessing_pipeline(): """Test CT preprocessing pipeline returns expected transform composition.""" pipeline = get_ct_preprocessing_pipeline() assert hasattr(pipeline, 'transforms') assert len(pipeline.transforms) == 3 assert isinstance(pipeline.transforms[0], LoadImage) assert isinstance(pipeline.transforms[1], EnsureChannelFirst) assert isinstance(pipeline.transforms[2], ScaleIntensityRange) + # Verify CT-specific HU window parameters + scale_transform = pipeline.transforms[2] + assert scale_transform.a_min == -1000 + assert scale_transform.a_max == 400 + assert scale_transform.b_min == 0.0 + assert scale_transform.b_max == 1.0 + assert scale_transform.clip is True
22-29: Consider verifying transform parameters.Test checks composition but not NormalizeIntensity parameters. Add assertion for
nonzeroparameter.def test_mri_preprocessing_pipeline(): """Test MRI preprocessing pipeline returns expected transform composition.""" pipeline = get_mri_preprocessing_pipeline() assert hasattr(pipeline, 'transforms') assert len(pipeline.transforms) == 3 assert isinstance(pipeline.transforms[0], LoadImage) assert isinstance(pipeline.transforms[1], EnsureChannelFirst) assert isinstance(pipeline.transforms[2], NormalizeIntensity) + # Verify MRI-specific normalization parameter + normalize_transform = pipeline.transforms[2] + assert normalize_transform.nonzero is True
📜 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/tests/test_clinical_preprocessing.py(1 hunks)monai/transforms/clinical_preprocessing.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:
monai/tests/test_clinical_preprocessing.pymonai/transforms/clinical_preprocessing.py
🧬 Code graph analysis (1)
monai/tests/test_clinical_preprocessing.py (1)
monai/transforms/clinical_preprocessing.py (5)
get_ct_preprocessing_pipeline(28-34)get_mri_preprocessing_pipeline(37-43)preprocess_dicom_series(46-77)UnsupportedModalityError(18-20)ModalityTypeError(23-25)
🪛 Ruff (0.14.8)
monai/transforms/clinical_preprocessing.py
64-64: Avoid specifying long messages outside the exception class
(TRY003)
73-75: 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: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- 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.11)
🔇 Additional comments (7)
monai/tests/test_clinical_preprocessing.py (3)
1-9: Imports look good.All necessary imports present for current tests.
32-35: LGTM - properly uses pytest.raises.Error handling test correctly implemented per previous feedback.
38-41: LGTM - properly uses pytest.raises.Type error handling test correctly implemented per previous feedback.
monai/transforms/clinical_preprocessing.py (4)
1-15: Module setup looks good.Imports and constants properly defined. MetaTensor import addresses previous review feedback.
17-25: Custom exceptions properly defined.Simple exception classes with descriptive docstrings.
46-62: Well-documented function signature.Complete Google-style docstring with Args, Returns, and Raises sections. Type hints properly defined.
63-77: Implementation logic is correct.Type checking, normalization, and modality dispatch work as expected. Transform application delegates file validation to LoadImage.
Static analysis flags TRY003 (inline exception messages), but this is common practice for descriptive errors and can be ignored.
…ecks, and full tests with successful execution
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
monai/tests/test_clinical_preprocessing.py (2)
13-43: Pipeline assertions are OK; consider tightening a couple parameter checks to reduce false positives.
Right now you only validate transform types (and for CT/MRI only one parameter each). If these pipelines are meant to be stable APIs, consider also asserting key ctor args, e.g.LoadImage(image_only=True)and (if relevant)EnsureChannelFirstconfiguration, so a future change doesn’t silently degrade behavior while keeping the same classes.
45-54: Exception tests look good; message match is a bit brittle to ordering.
If the implementation ever changes the presentation order of supported modalities (e.g., using a set), the regexCT, MR, MRIwill fail despite correct behavior—consider matching the tokens independently (or matching againstSUPPORTED_MODALITIES-derived formatting if exposed).
📜 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/tests/test_clinical_preprocessing.py(1 hunks)monai/transforms/clinical_preprocessing.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:
monai/transforms/clinical_preprocessing.pymonai/tests/test_clinical_preprocessing.py
🪛 Ruff (0.14.8)
monai/tests/test_clinical_preprocessing.py
65-65: Unused lambda argument: x
(ARG005)
78-78: Unused lambda argument: x
(ARG005)
- Remove test code from production module and implement proper preprocessing pipelines - Add custom exception classes (ModalityTypeError, UnsupportedModalityError) - Fix test file mocking to avoid Ruff ARG005 lint warnings (use Mock instead of lambda) - Add verification of key constructor arguments (LoadImage configuration) - Make error message matching robust to ordering changes - Add comprehensive docstrings and input validation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/tests/test_clinical_preprocessing.py (1)
53-64: Weak assertion withoroperator.The error message format guarantees both "PET" and "Unsupported modality" are present. Using
ormakes this assertion pass even if one is missing.- assert "PET" in error_message or "Unsupported modality" in error_message + assert "Unsupported modality" in error_message + assert "PET" in error_message
📜 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/tests/test_clinical_preprocessing.py(1 hunks)monai/transforms/clinical_preprocessing.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:
monai/tests/test_clinical_preprocessing.pymonai/transforms/clinical_preprocessing.py
🧬 Code graph analysis (2)
monai/tests/test_clinical_preprocessing.py (1)
monai/transforms/clinical_preprocessing.py (5)
get_ct_preprocessing_pipeline(21-40)get_mri_preprocessing_pipeline(43-62)preprocess_dicom_series(65-104)UnsupportedModalityError(16-18)ModalityTypeError(11-13)
monai/transforms/clinical_preprocessing.py (4)
monai/transforms/compose.py (1)
Compose(123-393)monai/transforms/io/array.py (1)
LoadImage(109-305)monai/transforms/utility/array.py (1)
EnsureChannelFirst(175-234)monai/transforms/intensity/array.py (2)
ScaleIntensityRange(959-1013)NormalizeIntensity(816-925)
🪛 Ruff (0.14.8)
monai/transforms/clinical_preprocessing.py
83-83: Avoid specifying long messages outside the exception class
(TRY003)
99-101: 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: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.6.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-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (6)
monai/transforms/clinical_preprocessing.py (3)
21-40: Pipeline implementation looks correct.CT HU window [-1000, 400] is appropriate for soft tissue. Docstring is complete with Returns and Note sections.
43-62: MRI pipeline implementation looks correct.NormalizeIntensity with
nonzero=Trueproperly handles background regions. Docstring follows Google style.
81-104: Logic is sound.Modality normalization (strip, uppercase, MRI→MR alias) and error handling are well implemented. The TRY003 static analysis hints about exception message length are stylistic preferences; current approach is clear and readable.
monai/tests/test_clinical_preprocessing.py (3)
13-32: Thorough pipeline composition test.Verifies transform types, HU window parameters, and LoadImage configuration.
35-50: Good MRI pipeline coverage.Validates transform composition and nonzero normalization parameter.
77-101: Mock-based tests cover modality variants well.Tests uppercase, lowercase, whitespace, and MRI→MR aliasing.
for more information, see https://pre-commit.ci
DCO Remediation Commit for Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com> I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 5394658 I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 24665aa I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 798f8af I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: b3a6ac2 I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: a446448 I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: d7f134c I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: ce7850f I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 01b88fa I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 821fc9a Signed-off-by: Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>
…endrasinhdata7/MONAI into clinical-dicom-preprocessing
- Remove MetaTensor import/type hint to fix CI failures - Fix weak assertions with OR operator (now separate assertions) - Add verification of all key constructor arguments - Use Mock instead of lambda to avoid lint warnings - Update docstrings and error handling - Address all previous review feedback
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 (2)
monai/tests/test_clinical_preprocessing.py(1 hunks)monai/transforms/clinical_preprocessing.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:
monai/tests/test_clinical_preprocessing.pymonai/transforms/clinical_preprocessing.py
🧬 Code graph analysis (1)
monai/tests/test_clinical_preprocessing.py (1)
monai/transforms/clinical_preprocessing.py (5)
get_ct_preprocessing_pipeline(20-39)get_mri_preprocessing_pipeline(42-61)preprocess_dicom_series(64-103)UnsupportedModalityError(15-17)ModalityTypeError(10-12)
🪛 Ruff (0.14.8)
monai/transforms/clinical_preprocessing.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
98-100: 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). (11)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
| This module provides preprocessing pipelines for different medical imaging modalities. | ||
| """ | ||
|
|
||
| from monai.transforms import Compose, LoadImage, EnsureChannelFirst, ScaleIntensityRange, NormalizeIntensity |
There was a problem hiding this comment.
Add return type hint and missing import.
Line 64 lacks a return type annotation. Per past review feedback and coding guidelines, preprocess_dicom_series should return MetaTensor, but the import is missing.
Apply this diff:
+from monai.data import MetaTensor
from monai.transforms import Compose, LoadImage, EnsureChannelFirst, ScaleIntensityRange, NormalizeIntensityThen update line 64:
-def preprocess_dicom_series(path: str, modality: str):
+def preprocess_dicom_series(path: str, modality: str) -> MetaTensor:And update the docstring Returns section (lines 73-74):
Returns:
- The preprocessed image data.
+ MetaTensor: The preprocessed image data with metadata.Also applies to: 64-64
🤖 Prompt for AI Agents
In monai/transforms/clinical_preprocessing.py around line 7 and line 64, add the
missing import for MetaTensor (from monai.data import MetaTensor) and update the
function signature for preprocess_dicom_series to include the return type hint
-> MetaTensor; also update the docstring Returns section (lines ~73-74) to state
that the function returns a MetaTensor instance instead of leaving it
unspecified. Ensure imports are sorted with existing imports and the docstring
phrasing matches the project's style.
Signed-off-by: Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>
DCO Remediation Commit for Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com> I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 5394658 I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 24665aa I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 798f8af I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: b3a6ac2 I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: a446448 I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: d7f134c I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: ce7850f I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 01b88fa I, Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>, hereby add my Signed-off-by to this commit: 821fc9a Signed-off-by: Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>
…endrasinhdata1/MONAI into clinical-dicom-preprocessing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/transforms/clinical_preprocessing.py (1)
76-98: Logic is correct - handles type validation, normalization, and pipeline selection properly.The string normalization (strip + uppercase) and MRI synonym mapping appropriately handle input variations.
Optional: Static analysis (Ruff TRY003) suggests defining exception messages in exception class constructors. Current approach provides better context and is acceptable.
📜 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/clinical_preprocessing.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:
monai/transforms/clinical_preprocessing.py
🧬 Code graph analysis (1)
monai/transforms/clinical_preprocessing.py (5)
monai/data/meta_tensor.py (1)
MetaTensor(52-609)monai/transforms/compose.py (1)
Compose(123-393)monai/transforms/io/array.py (1)
LoadImage(109-305)monai/transforms/utility/array.py (1)
EnsureChannelFirst(175-234)monai/transforms/intensity/array.py (2)
ScaleIntensityRange(959-1013)NormalizeIntensity(816-925)
🪛 Ruff (0.14.8)
monai/transforms/clinical_preprocessing.py
77-77: Avoid specifying long messages outside the exception class
(TRY003)
93-95: 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: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
🔇 Additional comments (5)
monai/transforms/clinical_preprocessing.py (5)
1-8: LGTM - Imports and module structure correct.Module docstring is clear. MetaTensor import addresses past review feedback.
11-18: LGTM - Exception classes appropriately defined.Exception hierarchy (TypeError for type, ValueError for unsupported value) is correct.
21-40: LGTM - CT pipeline correctly implements standard soft tissue windowing.The HU window [-1000, 400] is a reasonable general default. Pipeline composition is correct.
43-62: LGTM - MRI pipeline correctly handles non-zero normalization.Using
nonzero=Trueappropriately excludes background zeros from normalization statistics.
102-108: LGTM - Public API exports complete.All five public symbols are correctly exported.
Signed-off-by: Hitendrasinh Rathod <Hitendrasinh.data7@gmail.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
monai/transforms/clinical_preprocessing.py (3)
11-18: Consider default exception messages.Both exception classes use
passwith no default message. While not required, providing default messages can improve usability when raised without arguments.class ModalityTypeError(TypeError): - """Exception raised when modality parameter is not a string.""" - pass + """Exception raised when modality parameter is not a string.""" + def __init__(self, message="modality must be a string"): + super().__init__(message) class UnsupportedModalityError(ValueError): - """Exception raised when an unsupported modality is requested.""" - pass + """Exception raised when an unsupported modality is requested.""" + def __init__(self, message="Unsupported modality"): + super().__init__(message)
65-104: Static analysis hint: TRY003 is a low-priority style preference.Ruff suggests avoiding long exception messages outside the class definition. However, your messages include dynamic values (
modalityandtype(modality).__name__), making class-level constants impractical. This is acceptable.
93-96: Consider caching pipelines for performance.Pipelines are recreated on each call. For repeated preprocessing, caching improves performance. Optional enhancement:
Add module-level cached pipelines:
+# Cache pipelines at module level +_CT_PIPELINE = None +_MR_PIPELINE = None + def get_ct_preprocessing_pipeline() -> Compose: """...""" + global _CT_PIPELINE + if _CT_PIPELINE is None: + _CT_PIPELINE = Compose([ + LoadImage(image_only=True), + EnsureChannelFirst(), + ScaleIntensityRange(a_min=-1000, a_max=400, b_min=0.0, b_max=1.0, clip=True) + ]) + return _CT_PIPELINE - return Compose([ - LoadImage(image_only=True), - EnsureChannelFirst(), - ScaleIntensityRange(a_min=-1000, a_max=400, b_min=0.0, b_max=1.0, clip=True) - ])Apply similar pattern to
get_mri_preprocessing_pipeline.
📜 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/clinical_preprocessing.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:
monai/transforms/clinical_preprocessing.py
🪛 Ruff (0.14.8)
monai/transforms/clinical_preprocessing.py
83-83: Avoid specifying long messages outside the exception class
(TRY003)
99-101: 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.12)
- 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 (macOS-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: build-docs
- GitHub Check: packaging
🔇 Additional comments (7)
monai/transforms/clinical_preprocessing.py (7)
21-40: LGTM - CT pipeline is well-documented.The HU window [-1000, 400] is appropriate for soft tissue visualization. However, be aware different clinical applications (e.g., bone, lung) may require different windows. Consider documenting this limitation or exposing window parameters in future enhancements.
43-62: LGTM - MRI pipeline is well-documented.The
nonzero=Trueparameter correctly excludes background voxels from normalization statistics. Note that different MRI sequences may benefit from sequence-specific preprocessing strategies.
82-83: Type validation is correct.The type check and error message properly identify non-string inputs.
86-90: Input normalization is well-implemented.Stripping whitespace and uppercasing handles common input variations. The MRI→MR synonym mapping is documented and appropriate.
93-101: Pipeline selection logic is clear.The conditional logic correctly maps normalized modality strings to their respective pipelines and raises descriptive errors for unsupported values.
107-114: Public API export is complete.All public symbols are correctly listed in
__all__.
7-8: Imports are correct and valid.LoadImage with
image_only=Truereturns a MetaTensor as documented. DICOM series directories are supported via ITKReader, and individual file loading works through the reader selection mechanism. No changes needed.
1 participant
Changes
get_ct_preprocessing_pipeline()andget_mri_preprocessing_pipeline()with Google-style docstrings.preprocess_dicom_series()to include type hints and more informative exception messages.tests/test_clinical_preprocessing.pycovering:Motivation
These utilities help standardize preprocessing of clinical DICOM images and facilitate AI model development in medical imaging.
Tests
All tests have been added and verified locally using
pytest.