feat: update displayCTSegOverlay with axis inputs - #140
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 16 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
WalkthroughThe PR renames the Pixi config section from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/readii/image_processing.py (3)
215-265:⚠️ Potential issue | 🟠 Major
np.ndarrayinputs break the documented default paths.The signature now says
ctImage/segImagecan benp.ndarray, but Line 261 callsgetCroppedImages(...)and Line 265 callsgetROICenterCoords(segImage)before any normalization. Both helpers only handlesitk.Image, sodisplayCTSegOverlay(array_ct, array_seg)will fail for the defaultsliceIdx=-1case and forcrop=True. Please either normalize arrays up front or narrow the public type/doc contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/readii/image_processing.py` around lines 215 - 265, displayCTSegOverlay accepts np.ndarray but calls getCroppedImages and getROICenterCoords which only handle sitk.Image, causing failures when sliceIdx==-1 or crop=True; fix by normalizing inputs at the top of displayCTSegOverlay: detect np.ndarray for ctImage/segImage and convert them to sitk.Image (e.g., via sitk.GetImageFromArray) before any calls to getCroppedImages or getROICenterCoords so downstream helpers always receive sitk.Image, and ensure the converted images preserve expected axis order and spacing/origin metadata if required.
274-315:⚠️ Potential issue | 🔴 CriticalResolve the duplicated overlay block before merge.
This section currently has duplicated
dispMin/dispMaxinitialization, twoif ax is Noneblocks, duplicatedimshowcalls, and unmatched parentheses around the CT overlay. In its current state, the module will not import.Suggested cleanup
- if dispMin == None: - dispMin = ctImage.min() - if dispMax == None: - dispMax = ctImage.max() - - if dispMin == None: - dispMin = ctImage.min() - if dispMax == None: - dispMax = ctImage.max() + if dispMin is None: + dispMin = ctImage.min() + if dispMax is None: + dispMax = ctImage.max() # Make mask of ROI to ignore background in overlaid plot maskSeg = np.ma.masked_where(segImage == 0, segImage) - if ax is None: - # Create a new axis - fig, ax = plt.subplots() - if ax is None: - # Create a new axis - fig, ax = plt.subplots() + _, ax = plt.subplots() # Plot slice of CT - ax.imshow( - ctImage[sliceIdx, :, :], cmap=cmapCT, vmin=dispMin, vmax=dispMax - ax.imshow( - ctImage[sliceIdx, :, :], cmap=cmapCT, vmin=dispMin, vmax=dispMax - ) + ax.imshow(ctImage[sliceIdx, :, :], cmap=cmapCT, vmin=dispMin, vmax=dispMax) # Plot mask of ROI overtop - ax.imshow( - ax.imshow( + ax.imshow( maskSeg[sliceIdx, :, :], cmap=cmapSeg, vmin=segImage.min(), @@ ) ax.axis("off") return ax - ax.axis("off") - - return ax🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/readii/image_processing.py` around lines 274 - 315, The code contains duplicated initialization and plotting blocks causing syntax errors and incorrect flow: remove the repeated dispMin/dispMax checks (keep one each), keep a single "if ax is None: fig, ax = plt.subplots()" block, fix the CT imshow call to have matching parentheses and only one call (ax.imshow(ctImage[sliceIdx,:,:], cmap=cmapCT, vmin=dispMin, vmax=dispMax)), then render the mask once with ax.imshow(maskSeg[sliceIdx,:,:], cmap=cmapSeg, vmin=segImage.min(), vmax=segImage.max(), alpha=alpha), ensure ax.axis("off") appears before the single return ax, and delete any duplicated lines referencing maskSeg, segImage, or return.
104-116:⚠️ Potential issue | 🟠 Major
alignedSegImageis not actually a standalone alternative here.The new validation allows
padSegToMatchCT(..., alignedSegImage=...)withoutsegImagePath, but Line 116 still unconditionally callspydicom.dcmread(segImagePath, ...). That makes the advertised optional path fail at runtime. Either keepsegImagePathrequired, or accept the SEG metadata needed for slice mapping as a separate argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/readii/image_processing.py` around lines 104 - 116, The code unconditionally calls pydicom.dcmread(segImagePath, ...) even when segImagePath can be None and an alignedSegImage is provided to padSegToMatchCT; update padSegToMatchCT signature/usage to accept a segmentation header object (e.g., segHeader) or ensure segWithHeader is obtained from alignedSegImage metadata, and change this block to: if segImagePath is provided use pydicom.dcmread(segImagePath, stop_before_pixels=True) to set segWithHeader, else require/consume a passed-in segHeader (or extract header from alignedSegImage) before proceeding; adjust callers to pass the new segHeader argument when they supply alignedSegImage instead of a path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/readii/image_processing.py`:
- Around line 215-265: displayCTSegOverlay accepts np.ndarray but calls
getCroppedImages and getROICenterCoords which only handle sitk.Image, causing
failures when sliceIdx==-1 or crop=True; fix by normalizing inputs at the top of
displayCTSegOverlay: detect np.ndarray for ctImage/segImage and convert them to
sitk.Image (e.g., via sitk.GetImageFromArray) before any calls to
getCroppedImages or getROICenterCoords so downstream helpers always receive
sitk.Image, and ensure the converted images preserve expected axis order and
spacing/origin metadata if required.
- Around line 274-315: The code contains duplicated initialization and plotting
blocks causing syntax errors and incorrect flow: remove the repeated
dispMin/dispMax checks (keep one each), keep a single "if ax is None: fig, ax =
plt.subplots()" block, fix the CT imshow call to have matching parentheses and
only one call (ax.imshow(ctImage[sliceIdx,:,:], cmap=cmapCT, vmin=dispMin,
vmax=dispMax)), then render the mask once with ax.imshow(maskSeg[sliceIdx,:,:],
cmap=cmapSeg, vmin=segImage.min(), vmax=segImage.max(), alpha=alpha), ensure
ax.axis("off") appears before the single return ax, and delete any duplicated
lines referencing maskSeg, segImage, or return.
- Around line 104-116: The code unconditionally calls
pydicom.dcmread(segImagePath, ...) even when segImagePath can be None and an
alignedSegImage is provided to padSegToMatchCT; update padSegToMatchCT
signature/usage to accept a segmentation header object (e.g., segHeader) or
ensure segWithHeader is obtained from alignedSegImage metadata, and change this
block to: if segImagePath is provided use pydicom.dcmread(segImagePath,
stop_before_pixels=True) to set segWithHeader, else require/consume a passed-in
segHeader (or extract header from alignedSegImage) before proceeding; adjust
callers to pass the new segHeader argument when they supply alignedSegImage
instead of a path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ad31124c-0650-4faf-9556-6a1634783313
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.tomlsrc/readii/image_processing.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/readii/image_processing.py (1)
58-157: Remove the fully commentedpadSegToMatchCTimplementation from the module.This large commented block is dead code and obscures the active API surface. If this function is intentionally deprecated/disabled, remove it and track deprecation in changelog/docs instead of keeping the full body commented inline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/readii/image_processing.py` around lines 58 - 157, Remove the large commented-out implementation of padSegToMatchCT from src/readii/image_processing.py: delete the entire commented block referencing padSegToMatchCT and related helper calls (read_dicom_auto, loadSegmentation, flattenImage, alignImages, MedImage, Series, etc. as they appear in the block) so the module no longer contains dead/obscuring code; if this function was intentionally deprecated, add a short entry to the project changelog/docs noting its removal/deprecation instead of keeping the implementation commented inline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/readii/image_processing.py`:
- Around line 221-223: displayCTSegOverlay currently types accepts np.ndarray
but later calls functions that require sitk.Image (getCroppedImages,
getROICenterCoords), causing runtime failures; fix by detecting np.ndarray
inputs at the start of displayCTSegOverlay and converting them to sitk.Image (or
raising a clear error) before any calls to getCroppedImages or
getROICenterCoords, and ensure sliceIdx/crop logic operates on the converted
sitk.Image; reference the functions displayCTSegOverlay, getCroppedImages, and
getROICenterCoords when making the change.
- Around line 257-262: The docstring for displayCTSegOverlay has duplicate
parameter entries for dispMin, dispMax, and ax; remove the repeated parameter
blocks so each parameter appears exactly once, keeping the correct type and
concise description (e.g., dispMin : int, dispMax : int, ax : plt.Axes) in the
parameters section of displayCTSegOverlay and ensure the remaining docstring
follows the project's docstring style (numpy/google) and formatting.
- Line 368: The type hint for parameter segmentationLabel in getCroppedImages
conflicts with its None default; change its annotation from int to a nullable
type (e.g., Optional[int] or int | None) and add the necessary import (from
typing import Optional) if using Optional so the signature reflects that
segmentationLabel can be None; update any related docstring or callers if they
rely on a strict int type.
---
Nitpick comments:
In `@src/readii/image_processing.py`:
- Around line 58-157: Remove the large commented-out implementation of
padSegToMatchCT from src/readii/image_processing.py: delete the entire commented
block referencing padSegToMatchCT and related helper calls (read_dicom_auto,
loadSegmentation, flattenImage, alignImages, MedImage, Series, etc. as they
appear in the block) so the module no longer contains dead/obscuring code; if
this function was intentionally deprecated, add a short entry to the project
changelog/docs noting its removal/deprecation instead of keeping the
implementation commented inline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 974135e9-628a-4a88-8bbe-10b997584018
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/readii/image_processing.py
| ctImage: sitk.Image | np.ndarray, | ||
| segImage: sitk.Image | np.ndarray, | ||
| sliceIdx:int=-1, |
There was a problem hiding this comment.
displayCTSegOverlay declares np.ndarray support but runtime paths still require sitk.Image.
At Line 266, getCroppedImages(...) expects sitk.Image; at Line 270, getROICenterCoords(...) also assumes sitk.Image. With np.ndarray inputs, crop=True or sliceIdx=-1 will fail at runtime.
Proposed fix
def displayCTSegOverlay(
ctImage: sitk.Image | np.ndarray,
segImage: sitk.Image | np.ndarray,
@@
) -> plt.Axes:
@@
+ def _center_slice(seg: sitk.Image | np.ndarray) -> int:
+ segArr = sitk.GetArrayFromImage(seg) if isinstance(seg, sitk.Image) else seg
+ nz = np.argwhere(segArr != 0)
+ if nz.size == 0:
+ raise ValueError("segImage contains no ROI voxels; cannot infer center slice.")
+ return int(nz[len(nz) // 2, 0])
+
# If crop indicated, crop the CT and segmentation to just around the ROI
if crop:
+ if not isinstance(ctImage, sitk.Image) or not isinstance(segImage, sitk.Image):
+ raise TypeError("crop=True requires sitk.Image inputs for ctImage and segImage.")
ctImage, segImage = getCroppedImages(ctImage, segImage)
@@
# If slice index is not provided, get the center slice for the ROI in segImage
if sliceIdx == -1:
- sliceIdx, _, _ = getROICenterCoords(segImage)
+ sliceIdx = _center_slice(segImage)Also applies to: 265-270, 273-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/readii/image_processing.py` around lines 221 - 223, displayCTSegOverlay
currently types accepts np.ndarray but later calls functions that require
sitk.Image (getCroppedImages, getROICenterCoords), causing runtime failures; fix
by detecting np.ndarray inputs at the start of displayCTSegOverlay and
converting them to sitk.Image (or raising a clear error) before any calls to
getCroppedImages or getROICenterCoords, and ensure sliceIdx/crop logic operates
on the converted sitk.Image; reference the functions displayCTSegOverlay,
getCroppedImages, and getROICenterCoords when making the change.
| dispMin : int | ||
| Value to use as min for cmap in display | ||
| dispMax : int | ||
| Value to use as max for cmap in display | ||
| ax : plt.Axes | ||
| Axis to plot the slice on. If None, will create a new axis. |
There was a problem hiding this comment.
Duplicate docstring entries in displayCTSegOverlay.
dispMin, dispMax, and ax are documented twice, which makes the API docs noisy and error-prone to maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/readii/image_processing.py` around lines 257 - 262, The docstring for
displayCTSegOverlay has duplicate parameter entries for dispMin, dispMax, and
ax; remove the repeated parameter blocks so each parameter appears exactly once,
keeping the correct type and concise description (e.g., dispMin : int, dispMax :
int, ax : plt.Axes) in the parameters section of displayCTSegOverlay and ensure
the remaining docstring follows the project's docstring style (numpy/google) and
formatting.
|
|
||
| def getCroppedImages(ctImage, segImage, segmentationLabel=None): | ||
| """A function to crop a CT and segmentation to close to the ROI within the segmentation. | ||
| def getCroppedImages(ctImage: sitk.Image, segImage: sitk.Image, segmentationLabel:int=None)-> tuple[sitk.Image, sitk.Image]: |
There was a problem hiding this comment.
segmentationLabel type hint conflicts with its None default.
Line 368 annotates segmentationLabel as int but defaults to None. Use Optional[int] (or int | None) to match actual behavior.
Proposed fix
-def getCroppedImages(ctImage: sitk.Image, segImage: sitk.Image, segmentationLabel:int=None)-> tuple[sitk.Image, sitk.Image]:
+def getCroppedImages(
+ ctImage: sitk.Image,
+ segImage: sitk.Image,
+ segmentationLabel: Optional[int] = None,
+) -> tuple[sitk.Image, sitk.Image]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/readii/image_processing.py` at line 368, The type hint for parameter
segmentationLabel in getCroppedImages conflicts with its None default; change
its annotation from int to a nullable type (e.g., Optional[int] or int | None)
and add the necessary import (from typing import Optional) if using Optional so
the signature reflects that segmentationLabel can be None; update any related
docstring or callers if they rely on a strict int type.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 75.48% 76.15% +0.67%
==========================================
Files 41 41
Lines 2027 2009 -18
==========================================
Hits 1530 1530
+ Misses 497 479 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Also clean formatting of image_processing with ruff.
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor
Chores