Sogolsahebi/ feat/mri bias correction #432
Conversation
Removed redundant class definition and docstring for N4BiasFieldCorrection.
Removed unused import of mr_ from numpy.ma
📝 WalkthroughWalkthroughAdds an N4BiasFieldCorrection intensity transform and a bias_correction functional wrapper (SimpleITK). Re-exports the class from the transforms package and updates transformer logic to apply N4BiasFieldCorrection only when an image's metadata Modality equals "MR". Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (8)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #432 +/- ##
==========================================
- Coverage 54.81% 54.65% -0.16%
==========================================
Files 66 66
Lines 4302 4314 +12
==========================================
Hits 2358 2358
- Misses 1944 1956 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (3)
src/imgtools/transforms/transformer.py (1)
66-81: Consider reducing Transformer’s coupling to a specific transform classThe explicit
isinstance(transform, N4BiasFieldCorrection)check bakes N4-specific knowledge intoTransformer, while other transforms are handled via their base types and capabilities (e.g.,supports_reference). Longer term, you might improve maintainability by:
- Moving the modality gating into
N4BiasFieldCorrection.__call__(or a dedicatedsupports_modality(metadata)hook), so the transformer doesn’t need to know about concrete subclasses.- Or introducing a generic “requires MR modality” capability on transforms that N4 can implement.
This isn’t blocking, but keeping
Transformeragnostic to concrete transform types will make future additions easier to integrate and reason about.src/imgtools/transforms/intensity_transforms.py (2)
13-17: Export N4BiasFieldCorrection via__all__for consistency and discoverabilityThe new
N4BiasFieldCorrectionclass is implemented cleanly and its docstrings align with the behavior, but it’s not added to__all__. The other intensity transforms are exported there, so including N4 will make it easier to discover and to re-export from higher-level modules that rely on__all__.You can align it with the existing pattern like this:
__all__ = [ "IntensityTransform", "ClipIntensity", "WindowIntensity", + "N4BiasFieldCorrection", ]This keeps the public API consistent with the rest of the intensity transforms.
Also applies to: 155-180
155-180: Optionally make the internal SimpleITK filter explicit for clarityRight now,
self.filteris created in__post_init__without being declared on the dataclass, which works but is a bit implicit. If you want to improve readability and help static analysis tools, you could declare it as a non-init, non-repr field:-from dataclasses import dataclass +from dataclasses import dataclass, field @@ -@dataclass -class N4BiasFieldCorrection(IntensityTransform): - """ +@dataclass +class N4BiasFieldCorrection(IntensityTransform): + """ N4 bias-field correction. @@ - - def __post_init__(self) -> None: - self.filter = SimpleITKFilter(N4BiasFieldCorrectionImageFilter()) + + filter: SimpleITKFilter = field(init=False, repr=False) + + def __post_init__(self) -> None: + self.filter = SimpleITKFilter(N4BiasFieldCorrectionImageFilter())This keeps behavior the same while making the internal state and intent clearer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/imgtools/transforms/intensity_transforms.py(2 hunks)src/imgtools/transforms/transformer.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for compliance with PEP 8 and PEP 257 (docstring conventions). Ensure the following: - Variables and functions follow meaningful naming conventions. - Docstrings are present, accurate, and align with the implementation. - Code is efficient and avoids redundancy while adhering to DRY principles. - Consider suggestions to enhance readability and maintainability. - Highlight any potential performance issues, edge cases, or logical errors. - Ensure all imported libraries are used and necessary.
Files:
src/imgtools/transforms/transformer.pysrc/imgtools/transforms/intensity_transforms.py
🧬 Code graph analysis (1)
src/imgtools/transforms/transformer.py (1)
src/imgtools/transforms/intensity_transforms.py (1)
N4BiasFieldCorrection(157-179)
⏰ 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). (7)
- GitHub Check: Unit-Tests (macos-latest, py313, public)
- GitHub Check: Integration-Tests (ubuntu-latest, py310, public)
- GitHub Check: Integration-Tests (windows-latest, py313, public)
- GitHub Check: Integration-Tests (windows-latest, py310, public)
- GitHub Check: Integration-Tests (macos-latest, py313, public)
- GitHub Check: Integration-Tests (macos-latest, py310, public)
- GitHub Check: Integration-Tests (ubuntu-latest, py313, public)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/imgtools/transforms/intensity_transforms.py (1)
12-16: Add N4BiasFieldCorrection to the all list.The new
N4BiasFieldCorrectionclass is a public API addition but is missing from the__all__export list. This omission means the class won't be exported when usingfrom imgtools.transforms.intensity_transforms import *.Apply this diff to add the export:
__all__ = [ "IntensityTransform", "ClipIntensity", "WindowIntensity", + "N4BiasFieldCorrection", ]
🧹 Nitpick comments (1)
src/imgtools/transforms/intensity_transforms.py (1)
164-165: Consider adding configurability for N4 filter parameters.The current implementation uses default parameters for
N4BiasFieldCorrectionImageFilter. While this works well for typical cases, the filter supports configuration options like number of iterations and convergence threshold that may be useful for advanced use cases.If configurability is desired, consider this enhancement:
@dataclass class N4BiasFieldCorrection(IntensityTransform): """N4 bias field correction for MR images. ... + + Parameters + ---------- + number_of_iterations : list of int, optional + Number of iterations per resolution level. Default uses filter defaults. + convergence_threshold : float, optional + Convergence threshold. Default uses filter defaults. """ + number_of_iterations: list[int] | None = None + convergence_threshold: float | None = None def __post_init__(self) -> None: - self.filter = SimpleITKFilter(N4BiasFieldCorrectionImageFilter()) + n4_filter = N4BiasFieldCorrectionImageFilter() + if self.number_of_iterations is not None: + n4_filter.SetMaximumNumberOfIterations(self.number_of_iterations) + if self.convergence_threshold is not None: + n4_filter.SetConvergenceThreshold(self.convergence_threshold) + self.filter = SimpleITKFilter(n4_filter)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/imgtools/transforms/intensity_transforms.py(2 hunks)src/imgtools/transforms/transformer.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imgtools/transforms/transformer.py
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for compliance with PEP 8 and PEP 257 (docstring conventions). Ensure the following: - Variables and functions follow meaningful naming conventions. - Docstrings are present, accurate, and align with the implementation. - Code is efficient and avoids redundancy while adhering to DRY principles. - Consider suggestions to enhance readability and maintainability. - Highlight any potential performance issues, edge cases, or logical errors. - Ensure all imported libraries are used and necessary.
Files:
src/imgtools/transforms/intensity_transforms.py
⏰ 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). (7)
- GitHub Check: Integration-Tests (windows-latest, py310, public)
- GitHub Check: Integration-Tests (macos-latest, py313, public)
- GitHub Check: Integration-Tests (macos-latest, py310, public)
- GitHub Check: Integration-Tests (windows-latest, py313, public)
- GitHub Check: Integration-Tests (ubuntu-latest, py313, public)
- GitHub Check: Integration-Tests (ubuntu-latest, py310, public)
- GitHub Check: Linting
🔇 Additional comments (2)
src/imgtools/transforms/intensity_transforms.py (2)
3-3: LGTM! Imports are properly utilized.Both
N4BiasFieldCorrectionImageFilterandSimpleITKFilterare appropriately imported and used in the newN4BiasFieldCorrectionclass.Also applies to: 10-10
167-179: LGTM! Clean delegation pattern.The
__call__method properly delegates to the wrapped filter and includes a well-formatted docstring. The implementation correctly preserves image geometry while applying intensity correction.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/imgtools/transforms/functional.py (1)
418-433: Consider splitting the docstring summary line for PEP 257 compliance.The first line of the docstring is quite long. PEP 257 recommends a concise one-line summary followed by a blank line before detailed description. The other functions in this file (e.g.,
clip_intensity,window_intensity) follow this pattern.def bias_correction(image: sitk.Image) -> sitk.Image: - """Apply N4 bias field correction to reduce smooth intensity inhomogeneities (bias fields) commonly found in - MR imaging. This transform corrects voxel intensities while preserving image geometry (spacing, orientation, and dimensions). + """Apply N4 bias field correction to an MR image. + + Reduces smooth intensity inhomogeneities (bias fields) commonly found + in MR imaging. This transform corrects voxel intensities while preserving + image geometry (spacing, orientation, and dimensions). Parameters ----------src/imgtools/transforms/intensity_transforms.py (1)
177-189: Add blank line between docstring sections for consistency.The
__call__docstring is missing a blank line between theParametersandReturnssections, which differs from the formatting style used in other methods in this file.image : Image The input MR image to apply bias-field correction. + Returns -------
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/imgtools/transforms/__init__.py(2 hunks)src/imgtools/transforms/functional.py(3 hunks)src/imgtools/transforms/intensity_transforms.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for compliance with PEP 8 and PEP 257 (docstring conventions). Ensure the following: - Variables and functions follow meaningful naming conventions. - Docstrings are present, accurate, and align with the implementation. - Code is efficient and avoids redundancy while adhering to DRY principles. - Consider suggestions to enhance readability and maintainability. - Highlight any potential performance issues, edge cases, or logical errors. - Ensure all imported libraries are used and necessary.
Files:
src/imgtools/transforms/__init__.pysrc/imgtools/transforms/intensity_transforms.pysrc/imgtools/transforms/functional.py
🧬 Code graph analysis (3)
src/imgtools/transforms/__init__.py (1)
src/imgtools/transforms/intensity_transforms.py (1)
N4BiasFieldCorrection(154-189)
src/imgtools/transforms/intensity_transforms.py (1)
src/imgtools/transforms/functional.py (3)
bias_correction(418-433)clip_intensity(365-388)window_intensity(391-415)
src/imgtools/transforms/functional.py (1)
src/imgtools/transforms/lambda_transforms.py (1)
SimpleITKFilter(11-89)
🔇 Additional comments (4)
src/imgtools/transforms/__init__.py (1)
14-14: LGTM! Clean addition to the public API.The import and
__all__export follow the existing conventions in this file, maintaining alphabetical order within the import block and logical grouping under the intensity transform section.Also applies to: 46-46
src/imgtools/transforms/functional.py (1)
4-5: Import and export additions look good.The
SimpleITKFilterimport is properly utilized by the newbias_correctionfunction, and the export is correctly added to__all__.Also applies to: 18-18
src/imgtools/transforms/intensity_transforms.py (2)
153-172: Well-documented class with clear purpose.The docstring is comprehensive, covering the algorithm reference, computational considerations, and modality suitability. This addresses maintainability well by giving future developers the context they need.
6-6: Import and export properly configured.The
bias_correctionimport is used by the new class, and the__all__export is correctly updated.Also applies to: 12-12
| # def __post_init__(self) -> None: | ||
| # self.filter = SimpleITKFilter(N4BiasFieldCorrectionImageFilter()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
This commented-out __post_init__ block is dead code. If it's no longer needed, it should be removed to keep the codebase clean. If it's a placeholder for future work, consider opening an issue to track it instead.
- # def __post_init__(self) -> None:
- # self.filter = SimpleITKFilter(N4BiasFieldCorrectionImageFilter())
-
def __call__(self, image: Image) -> Image:📝 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.
| # def __post_init__(self) -> None: | |
| # self.filter = SimpleITKFilter(N4BiasFieldCorrectionImageFilter()) | |
| def __call__(self, image: Image) -> Image: |
🤖 Prompt for AI Agents
In src/imgtools/transforms/intensity_transforms.py around lines 174 to 176,
remove the commented-out __post_init__ block (the two lines that instantiate
SimpleITKFilter with N4BiasFieldCorrectionImageFilter) because it is dead code;
if the intent was to keep it as a reminder, either open a tracking issue or add
a TODO with a link to that issue, otherwise simply delete the commented lines to
clean up the codebase.
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)
src/imgtools/transforms/transformer.py (1)
96-113: Plainsitk.Imagepath returns an undefined name and may never be reachableUnrelated to the new N4 logic, but for maintainability it’s worth noting:
- In the
elsebranch (intended for plainsitk.Image), youreturn result, which is only defined in the precedingVectorMask/MedImagebranches, so this would raise aNameErrorif ever hit.- Combined with the N4 gating (and other transform logic) that accesses
transformed_image.metadata, a plainsitk.Imagewithoutmetadatawould already error earlier in the pipeline.If you intend to support raw
sitk.Imagehere, consider:- else: - # For plain sitk.Image, just return the transformed image - return result + else: + # For plain sitk.Image, just return the transformed image + return transformed_imageand pairing that with the more defensive metadata access suggested above. If raw
sitk.Imageisn’t a supported input, simplifying this branch and tightening the type hints would also improve clarity.
🧹 Nitpick comments (1)
src/imgtools/transforms/transformer.py (1)
14-14: N4 MR-only gating works but can be made more robust and readableNice job restricting
N4BiasFieldCorrectionto MR by checkingModality == "MR". To make this more maintainable and defensive, consider:
- Guarding metadata access with
getattr(..., "metadata", {}) or {}so this doesn’t blow up if a transform ever yields an object without ametadatadict.- Pulling out
modality = metadata.get("Modality")for clarity and logging a skip when modality isn’t MR.- Avoiding the nested
ifby early-continuefor non‑MR N4, then unifying the call site so both N4 (when MR) and other intensity/spatial transforms go through the sametransform(transformed_image)path.For example:
- elif isinstance( - transform, (IntensityTransform, SpatialTransform) - ): - # Apply N4BiasFieldCorrection only for MR images - if isinstance(transform, N4BiasFieldCorrection): - if ( - transformed_image.metadata.get( - "Modality", "Unknown" - ) - == "MR" - ): - transformed_image = transform(transformed_image) # type: ignore - else: - transformed_image = transform(transformed_image) + elif isinstance( + transform, (IntensityTransform, SpatialTransform) + ): + if isinstance(transform, N4BiasFieldCorrection): + metadata = getattr(transformed_image, "metadata", {}) or {} + modality = metadata.get("Modality") + if modality != "MR": + logger.debug( + "Skipping N4BiasFieldCorrection for non-MR modality %r", + modality, + ) + continue + + transformed_image = transform(transformed_image) # type: ignoreThis keeps the MR-only behavior, improves resilience to edge cases, and simplifies the control flow a bit.
Also applies to: 72-85
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/imgtools/transforms/transformer.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for compliance with PEP 8 and PEP 257 (docstring conventions). Ensure the following: - Variables and functions follow meaningful naming conventions. - Docstrings are present, accurate, and align with the implementation. - Code is efficient and avoids redundancy while adhering to DRY principles. - Consider suggestions to enhance readability and maintainability. - Highlight any potential performance issues, edge cases, or logical errors. - Ensure all imported libraries are used and necessary.
Files:
src/imgtools/transforms/transformer.py
🧬 Code graph analysis (1)
src/imgtools/transforms/transformer.py (1)
src/imgtools/transforms/intensity_transforms.py (1)
N4BiasFieldCorrection(154-189)
⏰ 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). (7)
- GitHub Check: Unit-Tests (macos-latest, py313, public)
- GitHub Check: Integration-Tests (macos-latest, py310, public)
- GitHub Check: Integration-Tests (windows-latest, py313, public)
- GitHub Check: Integration-Tests (macos-latest, py313, public)
- GitHub Check: Integration-Tests (ubuntu-latest, py313, public)
- GitHub Check: Integration-Tests (windows-latest, py310, public)
- GitHub Check: Integration-Tests (ubuntu-latest, py310, public)
Add N4 Bias Correction for MR:
N4BiasFieldCorrectiontransform using SimpleITK’sN4BiasFieldCorrectionImageFilter, applied only for MR images viatransformed_image.metadata["Modality"] == "MR".Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.