fix: handle errors caused by MIT2.0 update #124
Conversation
|
Warning Rate limit exceeded@strixy16 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes update the minimum version requirement for the "med-imagetools" dependency, refactor DICOM and RTSTRUCT loading to use new unified functions, adjust ROI handling to explicit lists, and update related tests and imports. Test expectations for ROI masks, image sizes, origins, and feature values are revised to match the new loading logic and data structures. Several unused imports are removed across notebooks, source, and test files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Loader
participant ImgTools
participant RTStruct
User->>Loader: loadRTSTRUCTSITK(rtstructPath, baseImageDirPath, roiNames)
Loader->>ImgTools: read_dicom_auto(baseImageDirPath)
Loader->>ImgTools: read_dicom_auto(rtstructPath, modality="RTSTRUCT")
loop for each ROI name
Loader->>RTStruct: get_mask_ndarray(roiName, reference=baseImage, size=baseImage.shape)
alt Mask found
Loader->>Loader: Convert ndarray to SimpleITK.Image
Loader->>Loader: Store mask in result dict
else Mask not found
Loader->>Loader: Store None in result dict
end
end
Loader-->>User: Return {roiName: mask or None}
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🧹 Nitpick comments (1)
tests/test_feature_extraction.py (1)
95-95: Useisinstance()instead of type comparison.Replace direct type comparison with
isinstance()for better Python practice.- assert type(actual) == collections.OrderedDict, \ + assert isinstance(actual, collections.OrderedDict), \🧰 Tools
🪛 Ruff (0.8.2)
95-95: Use
isandis notfor type comparisons, orisinstance()for isinstance checks(E721)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
pyproject.toml(1 hunks)src/readii/feature_extraction.py(2 hunks)src/readii/loaders.py(2 hunks)src/readii/process/images/crop.py(1 hunks)src/readii/utils/logging_config.py(1 hunks)tests/process/images/test_crop.py(2 hunks)tests/test_feature_extraction.py(3 hunks)tests/test_image_processing.py(2 hunks)tests/test_loaders.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_image_processing.py (1)
tests/test_loaders.py (1)
lung4DCTPath(13-14)
tests/test_feature_extraction.py (1)
tests/test_loaders.py (1)
lung4DCTPath(13-14)
🪛 Ruff (0.8.2)
tests/test_feature_extraction.py
95-95: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
src/readii/loaders.py
19-19: imgtools.coretypes.RTStructureSet imported but unused
Remove unused import: imgtools.coretypes.RTStructureSet
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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-14, py312)
- GitHub Check: Unit-Tests (macos-14, py311)
- GitHub Check: Unit-Tests (macos-14, py310)
- GitHub Check: Unit-Tests (macos-latest, py311)
- GitHub Check: Unit-Tests (macos-latest, py310)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
🔇 Additional comments (13)
src/readii/utils/logging_config.py (1)
1-1: Updated import path correctly.The import path has been properly updated from
imgtools.loggingtoimgtools.loggersto align with the MIT2.0 package structure changes.pyproject.toml (1)
12-12: Dependency version update reflects MIT2.0 requirements.The minimum required version for med-imagetools was correctly updated from ">=1.9.2" to ">=2" to support the new API changes used throughout the codebase.
src/readii/process/images/crop.py (1)
5-5: Updated import path correctly.The import path has been properly updated from
imgtools.ops.functionaltoimgtools.transforms.functionalto reflect the MIT2.0 package structure changes.src/readii/feature_extraction.py (1)
8-8: Updated import path correctly.The import has been properly updated from
imgtools.io.read_dicom_seriestoimgtools.io.readers.read_dicom_autoto align with the MIT2.0 package restructuring.tests/test_image_processing.py (2)
21-22: ROI specification updated to reflect new MIT2.0 pattern.The change from a dictionary with regex pattern to a direct list of ROI names aligns with the MIT2.0 update, which requires explicit ROI specification rather than pattern matching.
61-61: Adjusted expected center coordinates to match updated ROI handling.The coordinates update from
(63, 314, 318)to(63, 312, 328)reflects changes in how the new MIT2.0 methods process and position the ROI masks.tests/test_loaders.py (2)
81-81: Updated image spacing expectations to match MIT2.0 output.The spacing values changed from
(0.9766, 0.9766, 3.0)to(1.0, 1.0, 1.0), reflecting the normalization applied by the updated MIT2.0 methods.
83-83: Origin reset in MIT2.0 reflected in test expectations.The origin values changed from specific coordinates to
(0, 0, 0), indicating that the new MIT2.0 implementation normalizes the origin.tests/process/images/test_crop.py (2)
40-40: ROI specification updated to explicit list format.Changed from a regex string to a direct list
["Tumor_c40"]to align with the MIT2.0 implementation's requirement for explicit ROI names.
46-46: Updated expected dimensions to match MIT2.0 cropping behavior.The expected dimensions have been incremented (e.g., from 92 to 93 for the cube method) to align with the updated MIT2.0 implementation which produces slightly larger cropped regions.
Also applies to: 57-59
tests/test_feature_extraction.py (3)
40-41: ROI specification updated to explicit list format.Changed from a dictionary mapping with regex to a direct list of ROI names, aligning with the MIT2.0 implementation. Correspondingly, the dictionary key used to access the segmentation is updated to the actual ROI name.
101-103: Updated expected size and volume metrics to match MIT2.0 output.The dimensions and volume calculations differ with the new MIT2.0 implementation. Expected size changed from a smaller bounding box to
(52, 93, 28)and the volume increased from ~66K to ~71K, reflecting the more accurate segmentation processing.Also applies to: 107-107
146-146: Standardized ROI name format as a list.Changed from a string to a list
["Tumor_c40"]for consistency with the MIT2.0 API requirements for ROI specifications.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/readii/loaders.py (2)
44-46:⚠️ Potential issueUpdate function signature for ROI names parameter
To match the implementation which iterates over
roiNames, the parameter type should be updated.def loadRTSTRUCTSITK( - rtstructPath: str | Path, baseImageDirPath: str | Path, roiNames: Optional[str] = None + rtstructPath: str | Path, baseImageDirPath: str | Path, roiNames: Optional[Sequence[str]] = None ) -> Dict[str, sitk.Image]:
93-97:⚠️ Potential issueUpdate
loadSegmentationfunction signatureThe
roiNamesparameter in this function should also be updated to match the implementation inloadRTSTRUCTSITK.def loadSegmentation( segImagePath: str | Path, modality: str, baseImageDirPath: Optional[str | Path] = None, - roiNames: Optional[str] = None, + roiNames: Optional[Sequence[str]] = None, ) -> Dict[str, sitk.Image]:
♻️ Duplicate comments (3)
src/readii/loaders.py (3)
16-18: Updated imports for MIT2.0 compatibilityThe switch to
read_dicom_autofrom the new MIT2.0 API allows for unified DICOM reading. However, you should remove the commented out import and unused imports to maintain clean code.from imgtools.io.readers import read_dicom_auto -# from imgtools.ops import StructureSetToSegmentation
70-71: 🛠️ Refactor suggestionAdded path existence validation needed
While the new
read_dicom_autofunction simplifies the code, it's recommended to add path existence checks to provide more meaningful error messages.logger.debug(f"Loading RTSTRUCT from directory: {rtstructPath}") +if not baseImageDirPath.exists(): + raise FileNotFoundError(f"Base-image directory not found: {baseImageDirPath}") +if not rtstructPath.exists(): + raise FileNotFoundError(f"RTSTRUCT file not found: {rtstructPath}") baseImage = read_dicom_auto(path=baseImageDirPath.resolve()) segImage = read_dicom_auto(path=rtstructPath.resolve(), modality="RTSTRUCT")
76-88: 🛠️ Refactor suggestionROI mask processing updated for MIT2.0
The updated implementation correctly uses the new MIT2.0 API for mask generation. However, there are several improvements needed:
- The function still declares
roiNamesasOptional[str]but iterates over it- No validation for empty/None
roiNames- The SimpleITK image created from the mask does not copy the spatial metadata from the base image
from pathlib import Path -from typing import Dict, Optional +from typing import Dict, Optional, Sequence def loadRTSTRUCTSITK( - rtstructPath: str | Path, baseImageDirPath: str | Path, roiNames: Optional[str] = None + rtstructPath: str | Path, baseImageDirPath: str | Path, roiNames: Optional[Sequence[str]] = None ) -> Dict[str, sitk.Image]: """Load RTSTRUCT into SimpleITK Image. ... roiNames : Sequence[str], optional Identifiers for which region(s) of interest to load from the segmentation file. ... """And in the implementation:
logger.debug(f"Making mask using ROI names: {roiNames}") +if not roiNames: + raise ValueError("`roiNames` must be a non-empty sequence of ROI names") img_size = baseImage.size segMasks = {} for roi in roiNames: try: mask_ndarray = segImage.get_mask_ndarray(reference_image = baseImage, roi_name = roi, mask_img_size = [img_size.depth, img_size.height, img_size.width, 1], continuous = False) - segMasks[roi] = sitk.GetImageFromArray(mask_ndarray) + mask = sitk.GetImageFromArray(mask_ndarray) + # Copy spatial metadata so the mask aligns with the base image + if hasattr(baseImage, 'sitk_image'): + mask.CopyInformation(baseImage.sitk_image) + else: + # If baseImage is already a SimpleITK image + mask.CopyInformation(baseImage) + segMasks[roi] = mask except ValueError: segMasks[roi] = None
🧹 Nitpick comments (2)
tests/test_feature_extraction.py (1)
94-94: Useisinstance()for type checkingType comparison with
==is less robust than usingisinstance().- assert type(actual) == collections.OrderedDict, \ + assert isinstance(actual, collections.OrderedDict), \🧰 Tools
🪛 Ruff (0.8.2)
94-94: Use
isandis notfor type comparisons, orisinstance()for isinstance checks(E721)
src/readii/loaders.py (1)
74-74: Log message needs to be updated for list of ROI namesThe log message should be updated to better represent that
roiNamesis now expected to be a sequence.-logger.debug(f"Making mask using ROI names: {roiNames}") +logger.debug(f"Making masks for ROIs: {', '.join(roiNames) if roiNames else 'None'}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
notebooks/crop_testing.ipynb(2 hunks)notebooks/debugging.ipynb(0 hunks)notebooks/viz_neg_4D.ipynb(0 hunks)notebooks/viz_neg_controls.ipynb(1 hunks)notebooks/writer_examples.ipynb(0 hunks)src/readii/loaders.py(2 hunks)src/readii/negative_controls.py(1 hunks)src/readii/pipeline.py(0 hunks)src/readii/process/images/crop.py(2 hunks)tests/analyze/test_plot_correlation.py(5 hunks)tests/io/loaders/test_features.py(0 hunks)tests/io/writers/test_base_writer.py(0 hunks)tests/io/writers/test_correlation_writer.py(1 hunks)tests/io/writers/test_nifti_writer.py(0 hunks)tests/process/images/test_crop.py(2 hunks)tests/test_feature_extraction.py(3 hunks)tests/test_negative_controls.py(1 hunks)
💤 Files with no reviewable changes (7)
- tests/io/loaders/test_features.py
- tests/io/writers/test_base_writer.py
- tests/io/writers/test_nifti_writer.py
- notebooks/writer_examples.ipynb
- notebooks/viz_neg_4D.ipynb
- src/readii/pipeline.py
- notebooks/debugging.ipynb
✅ Files skipped from review due to trivial changes (5)
- tests/io/writers/test_correlation_writer.py
- src/readii/negative_controls.py
- tests/analyze/test_plot_correlation.py
- notebooks/viz_neg_controls.ipynb
- notebooks/crop_testing.ipynb
🚧 Files skipped from review as they are similar to previous changes (2)
- src/readii/process/images/crop.py
- tests/process/images/test_crop.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_negative_controls.py (1)
src/readii/negative_controls.py (1)
negativeControlNonROIOnly(258-322)
tests/test_feature_extraction.py (1)
tests/test_loaders.py (1)
lung4DCTPath(13-14)
🪛 Ruff (0.8.2)
tests/test_feature_extraction.py
94-94: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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-14, py312)
- GitHub Check: Unit-Tests (macos-14, py311)
- GitHub Check: Unit-Tests (macos-latest, py311)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
🔇 Additional comments (4)
tests/test_negative_controls.py (1)
4-10: LGTM: Import cleanup aligned with MIT2.0 updateThe removal of unused imports (
imageoperationsfrom radiomics,applyNegativeControlfrom readii.negative_controls, andcollections) while keeping the necessary imports is a good cleanup. This aligns with the overall PR objective of fixing errors caused by the MIT2.0 update and streamlining imports.tests/test_feature_extraction.py (3)
39-40: Updated ROI specification formatThe change from dictionary/regex pattern
{'GTV':'Tumor_c.*'}to explicit list['Tumor_c40']aligns with the new MIT2.0 approach for ROI specification. This is more explicit and less error-prone than using regex patterns.
100-106: Size and volume modifications match new loader implementationThe updated expected dimensions (52x93x28) and volume feature (71110.67) reflect the changes in how the segmentation masks are extracted with the new MIT2.0 implementation. These values are essential to maintain test validity after refactoring.
145-145: Consistent ROI names formatThe change from string
"Tumor_c40"to list["Tumor_c40"]ensures consistent ROI specification across the codebase, improving maintainability.
1 participant
Updating import paths for the logger, io, resize, etc.
Updated RTSTRUCT loader to use new MIT setup
Updated some image readers to use read_dicom_auto
Updated tests for loading RTSTRUCTS with new MIT methods
Summary by CodeRabbit
Refactor
Tests
Chores