feat: Refactor image utility functions and enhance documentation #154
Conversation
…mproved documentation
… type annotations
WalkthroughThis pull request introduces comprehensive documentation and type improvements across several modules in the Changes
Sequence DiagramsequenceDiagram
participant User
participant StructureSet
participant DICOM
User->>StructureSet: from_dicom_rtstruct(path)
StructureSet->>DICOM: Read RTSTRUCT file
DICOM-->>StructureSet: Return ROI data
StructureSet->>StructureSet: Extract ROI points
StructureSet-->>User: Return StructureSet instance
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/imgtools/utils/imageutils.py (2)
1-2: Use direct imports for clarity.Currently, you're importing
ListandTuplefrom the typing module. Consider directly usinglistandtuplein modern Python (3.9+), which supports these as type hints without needingfrom typing import List, Tuple.
36-92: Vectorized transformation may cause high memory usage for large point sets.Using
np.vectorizeon lists of 3D points can simplify code, but be cautious about performance and memory usage ifpointsbecomes large. For extremely large sets, consider a streaming/batched approach to avoid potential memory overhead.src/imgtools/modules/segmentation.py (1)
66-112: Decorator logic is clear; consider surfacing transformation details in docstring.
accepts_segmentationspreserves Segmentation ROI indices and raw ROI names, which is excellent. You might consider clarifying in the docstring that the function automatically casts output toVectorUInt8.src/imgtools/modules/structureset.py (2)
266-267: Refactor the pattern iteration logic
There's aTODOcomment about refactoring this logic to use a generator for better readability. Refactoring_assign_labelsinto smaller, dedicated functions or generators could reduce complexity and improve testability.
329-329: Improve exception re-raising style
When re-raising exceptions inside anexceptblock, it’s considered best practice to useraise ... from Noneorraise ... from errto clarify the exception chain. This helps differentiate between the original error and subsequent handling errors.Possible minimal changes:
- raise IndexError(f'{z} index is out of bounds for image sized {mask.shape}.') + raise IndexError(f'{z} index is out of bounds for image sized {mask.shape}.') from None - raise ValueError('This contour is corrupted and spans across 2 or more slices.') + raise ValueError('This contour is corrupted and spans across 2 or more slices.') from NoneAlso applies to: 337-337
🧰 Tools
🪛 Ruff (0.8.2)
329-329: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/modules/test_structureset.py (1)
137-139: Duplicate docstring
Lines 137 and 139 both appear to declare the same docstring, creating redundancy. Removing the duplicate improves clarity.def test_from_dicom_rtstruct(mock_dcmread): - """Test from_dicom_rtstruct method with mocked DICOM file.""" """Test from_dicom_rtstruct method with mocked DICOM file."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/mypy.ini(1 hunks)config/ruff.toml(3 hunks)src/imgtools/modules/segmentation.py(1 hunks)src/imgtools/modules/structureset.py(1 hunks)src/imgtools/utils/imageutils.py(1 hunks)tests/modules/test_structureset.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/imgtools/modules/segmentation.py
56-56: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
tests/modules/test_structureset.py
4-4: typing.Dict imported but unused
Remove unused import
(F401)
4-4: typing.List imported but unused
Remove unused import
(F401)
5-5: pydicom.dataset.Dataset imported but unused
Remove unused import: pydicom.dataset.Dataset
(F401)
src/imgtools/modules/structureset.py
323-323: Do not use bare except
(E722)
329-329: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
337-337: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (9)
src/imgtools/utils/imageutils.py (2)
11-33: Comprehensive docstring and accurate return types.
The image_to_array function provides a clear docstring and robust type hints, ensuring a strong contract for users. This is a best practice for maintainability and clarity.
94-117: Good docstring; consider adding edge-case handling.
The idxs_to_physical_points function provides thorough documentation. If idxs is empty or has an unexpected shape, consider adding input validation or error handling to improve robustness.
src/imgtools/modules/segmentation.py (2)
1-50: Excellent top-level documentation.
The module-level docstring thoroughly describes the purpose, classes, and functions. This is a great practice to help new contributors understand the functionality quickly.
114-172: Well-organized function with thorough parameter documentation.
map_over_labels sets a high documentation standard by detailing behavior, usage, and examples. The step of optionally returning a new Segmentation object is especially clear.
config/mypy.ini (1)
6-7: Good expansion of type-checking coverage.
Including src/imgtools/modules/**/*.py broadens the type coverage of your project. This ensures new modules, such as those in segmentation.py, are validated by mypy.
config/ruff.toml (3)
7-8: Strategic inclusion for segmentation lint checks.
Adding src/imgtools/modules/segmentation.py to the include list ensures coverage under linting rules. This complements the mypy inclusion for thorough code quality control.
24-24: Extend-include reformatting looks consistent.
No issues detected. The single line for extend-include is structurally coherent and maintains readability.
105-106: Newly ignored rule EM101.
Adding EM101 to the ignore list can be helpful if your team has a specific style or pattern for error messages. Confirm that this rule’s suppression aligns with your project’s best practices.
src/imgtools/modules/structureset.py (1)
1-29: Comprehensive module docstring
The high-level overview and usage examples provide clear context for the StructureSet class. This thorough documentation enhances maintainability and helps new contributors quickly understand the module's purpose.
| from __future__ import annotations | ||
|
|
||
| import warnings | ||
| from functools import wraps | ||
| from typing import Any, Callable, Dict, List, Optional, Tuple, Union |
There was a problem hiding this comment.
Remove unused Dict import.
Static analysis indicates that typing.Dict is never referenced. Removing unused imports helps keep the code clean and reduces confusion.
- from typing import Any, Callable, Dict, List, Optional, Tuple, Union
+ from typing import Any, Callable, List, Optional, Tuple, Union📝 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.
| from __future__ import annotations | |
| import warnings | |
| from functools import wraps | |
| from typing import Any, Callable, Dict, List, Optional, Tuple, Union | |
| from __future__ import annotations | |
| import warnings | |
| from functools import wraps | |
| from typing import Any, Callable, List, Optional, Tuple, Union |
🧰 Tools
🪛 Ruff (0.8.2)
56-56: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
| except: # rounding errors for points on the boundary | ||
| if z == mask.shape[0]: |
There was a problem hiding this comment.
Avoid bare except
Catching all exceptions masks the root cause and can make debugging harder. To address this, catch specific exception types or use except Exception as e:. Additionally, consider chaining the exception properly to preserve its traceback.
Below is a suggested fix:
- except: # rounding errors for points on the boundary
+ except Exception as exc:
+ logger.debug("Caught an unexpected exception for boundary rounding", error=exc)
if z == mask.shape[0]:
...Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
323-323: Do not use bare except
(E722)
| from typing import Dict, List | ||
| from pydicom.dataset import Dataset |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports
Dict, List, and Dataset imports are unused, per static analysis hints. Removing them helps keep the file clean and avoids confusion.
-from typing import Dict, List
-from pydicom.dataset import Dataset
+from typing import AnyCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
4-4: typing.Dict imported but unused
Remove unused import
(F401)
4-4: typing.List imported but unused
Remove unused import
(F401)
5-5: pydicom.dataset.Dataset imported but unused
Remove unused import: pydicom.dataset.Dataset
(F401)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 44.03% 44.82% +0.78%
==========================================
Files 48 48
Lines 3540 3558 +18
==========================================
+ Hits 1559 1595 +36
+ Misses 1981 1963 -18 ☔ View full report in Codecov by Sentry. |
Improve type annotations, error messages, and documentation across various modules, while updating mypy and ruff configurations for better type checking and linting. Add tests for the StructureSet class to ensure reliability.
Summary by CodeRabbit
New Features
segmentation.pyandstructureset.pymodules, detailing class functionalities and methods.StructureSetclass for DICOM RTSTRUCT file handling.imageutils.py.Bug Fixes
Tests
StructureSetclass, covering various scenarios and edge cases.