feat: Add ImageData/MaskData classes to compute statistics #226
Conversation
📝 WalkthroughWalkthroughThe changes modify the behavior in four modules. In the box module, the Changes
Suggested labels
Constructive Feedback
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (5)
src/imgtools/coretypes/image_statistics.py (2)
39-49: Consider renaming attributes that match built-in function names.
Attributes likemin,max, andsumovershadow Python built-ins, which can cause confusion. Renaming them tomin_value,max_value, andintensity_sum(or similar) would enhance clarity.- min: float - max: float - sum: float + min_value: float + max_value: float + intensity_sum: float
217-263: Separate demonstration code from production code.
Thismain()function is helpful for illustrating usage, but consider moving it to an example script or Jupyter notebook. That way, production modules remain focused on reusable logic.src/imgtools/utils/imageutils.py (1)
187-187: Consider performance implications of the list comprehension.
The vectorized approach was replaced with a list comprehension, which is more readable but might be slower for very large arrays. If performance becomes critical, you could revisit vectorization or other batch-processing solutions.src/imgtools/coretypes/box.py (2)
138-140: Add a clarifying comment for zero-size box initialization.The code creates a box with identical min and max coordinates, which results in a zero-size box that will be expanded later. A comment explaining this would improve readability.
return RegionBox( + # Initialize with zero size, will be expanded by expand_to_cube Coordinate3D(*centroid_idx), Coordinate3D(*centroid_idx) ).expand_to_cube(desired_size)
115-130: Enhance docstring to explain box initialization and expansion.The docstring could better explain the method's behavior by mentioning that it creates a zero-size box at the centroid and always expands it to a cube.
"""Creates a RegionBox from the centroid of a mask image. Parameters ---------- mask : sitk.Image The input mask image. label : int, optional label in the mask image to calculate the centroid. desired_size : int | None, optional The desired size of the box. If None, the minimum size default from `expand_to_min_size` is used. Returns ------- RegionBox The bounding box coordinates as a RegionBox object. + + Notes + ----- + The box is initially created with zero size at the centroid and then + expanded to a cube using `expand_to_cube`. This ensures that the box + is centered on the centroid with equal dimensions. """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (4)
src/imgtools/coretypes/box.py(1 hunks)src/imgtools/coretypes/image_statistics.py(1 hunks)src/imgtools/coretypes/spatial_types.py(2 hunks)src/imgtools/utils/imageutils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.py`: Review the Python code for compliance with PE...
src/**/*.py: 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.
src/imgtools/utils/imageutils.pysrc/imgtools/coretypes/spatial_types.pysrc/imgtools/coretypes/box.pysrc/imgtools/coretypes/image_statistics.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Unit-Tests (windows-latest, py313)
- GitHub Check: Unit-Tests (windows-latest, py312)
- GitHub Check: Unit-Tests (windows-latest, py311)
- GitHub Check: Unit-Tests (windows-latest, py310)
- GitHub Check: Unit-Tests (macos-13, py313)
- GitHub Check: Unit-Tests (macos-13, py312)
- GitHub Check: Unit-Tests (macos-13, py311)
- GitHub Check: Unit-Tests (macos-13, py310)
- GitHub Check: Unit-Tests (macos-latest, py310)
- GitHub Check: Unit-Tests (ubuntu-latest, py313)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
- GitHub Check: Unit-Tests (ubuntu-latest, py310)
🔇 Additional comments (7)
src/imgtools/coretypes/image_statistics.py (4)
10-37: Docstrings look clear and comprehensive.
They accurately describe the purpose and usage of theImageDataclass and its attributes in a concise way.
50-81: Confirm or constrain the supported image dimensions.
This method computes statistics from any dimensional SimpleITK image. If the application only supports 3D images, consider adding a check or clarifying it in the docstring.Would you like to verify how this function behaves for non-3D images?
83-127: Thorough documentation forMaskData.
The docstring is well-structured and clearly explains each attribute, making it easy to track mask-specific statistics.
128-146: Consistent attribute naming.
The attributes related to masked statistics avoid overshadowing built-ins, improving readability. Good work!src/imgtools/utils/imageutils.py (1)
172-174: New comment clarifies the expected data type.
Explicitly documenting that a list is required to transform indices helps avoid confusion and potential type errors. Nicely done.src/imgtools/coretypes/spatial_types.py (1)
162-186: Precise__repr__enhances debugging.
Formatting spacing to five decimal places improves clarity. If further precision is needed (e.g., medical imaging requiring high accuracy), document and confirm the rationale. Otherwise, this is a solid implementation.src/imgtools/coretypes/box.py (1)
1-492: Implementation looks solid!The changes to create distinct coordinate objects in
from_mask_centroidare well-implemented. The overall code is well-documented, has good error handling, and follows best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/imgtools/coretypes/image_statistics.py (1)
147-211:⚠️ Potential issueCheck for empty masked_array_nonzero using its size, not None.
Line 180 checks forNone, butmasked_array[masked_array != 0]returns an empty array when there are no non-zero pixels, neverNone. This can lead to.max()or.min()calls on an empty array, causing runtime errors. Update the condition to handle empty arrays correctly.Here’s a suggested fix:
-if (masked_array_nonzero := masked_array[masked_array != 0]) is None: - msg = f"No non-zero values found in masked region with {label=}." - raise ValueError(msg) +masked_array_nonzero = masked_array[masked_array != 0] +if masked_array_nonzero.size == 0: + raise ValueError( + f"No non-zero values found in masked region with {label=}." + )
🧹 Nitpick comments (2)
src/imgtools/coretypes/image_statistics.py (2)
10-49: Consider renamingminandmaxto avoid overshadowing built-ins.
Usingminandmaxas attribute names overshadows Python’s built-in functions. Prefer something likeintensity_minandintensity_maxfor clarity and to uphold coding best practices.
214-261: Add a docstring or move demonstration code out of production.
While it’s valuable to illustrate howImageDataandMaskDatawork, consider adding a docstring or isolating this demonstration in a dedicated example script to keep library code lean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/imgtools/coretypes/image_statistics.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.py`: Review the Python code for compliance with PE...
src/**/*.py: 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.
src/imgtools/coretypes/image_statistics.py
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Unit-Tests (windows-latest, py313)
- GitHub Check: Unit-Tests (windows-latest, py312)
- GitHub Check: Unit-Tests (windows-latest, py311)
- GitHub Check: Unit-Tests (windows-latest, py310)
- GitHub Check: Unit-Tests (macos-13, py313)
- GitHub Check: Unit-Tests (macos-13, py312)
- GitHub Check: Unit-Tests (macos-13, py311)
- GitHub Check: Unit-Tests (macos-13, py310)
- GitHub Check: Unit-Tests (macos-latest, py310)
- GitHub Check: Unit-Tests (ubuntu-latest, py313)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
- GitHub Check: Unit-Tests (ubuntu-latest, py311)
- GitHub Check: Unit-Tests (ubuntu-latest, py310)
🔇 Additional comments (3)
src/imgtools/coretypes/image_statistics.py (3)
1-8: All good with the imports.
These imports, includingfrom __future__ import annotations, align well with modern Python practices.
50-81: ImageData.from_image seems correct.
The usage ofStatisticsImageFilterand returning a populatedImageDatais straightforward and readable. Nice job!
83-146: MaskData design is logically consistent.
The data class fields and their docstrings properly describe the mask-related statistics. This fosters maintainability and clear data handling.
jjjermiah
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #226 +/- ##
============================================
Coverage 65.12% 65.13%
============================================
Files 55 55
Lines 3710 3711 +1
============================================
+ Hits 2416 2417 +1
Misses 1294 1294 ☔ View full report in Codecov by Sentry. |
imgtools.Segmentationonce feat: add mask statistics getter functions in Segmentation #231 gets mergedview example as objects
Image Data
Notes on the mask
I used the following code to get the mask:
which is why we wee
volume_count=2belowMask Data
mask_stats(cropped to exactly bbox)
view example as dictionaries
Summary by CodeRabbit
New Features
Bug Fixes
Spacing3Dclass to display values with five decimal places.