CCHMC CT Seg Example App Updates#580
Open
bluna301 wants to merge 6 commits into
Open
Conversation
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves MongoDB outputs and restructures the example app into an end-to-end pipeline: series selection → DICOM series-to-volume (with nvimgcodec) → inference (MetaTensor) → per-organ metrics → percentile/z-score (optional PDF) → contour/overlay → DICOM SEG/SR/SC and PDF writing; updates docs, assets, and example packaging requirements. ChangesCCHMC Pediatric CT Liver-Spleen Segmentation
I/O, decoding & volume conversion
DICOM writers & SR
Z-score & visualization
Sequence Diagram(s)sequenceDiagram
participant User
participant App as AIAbdomenSegApp
participant Selector as DICOMSeriesSelector
participant Converter as DICOMSeriesToVolume
participant Inference as AbdomenSegOperator
participant Metrics as SegmentationMetricsOperator
participant ZScore as SegmentationZScoreOperator
participant Contour as SegmentationContourOperator
participant Overlay as SegmentationOverlayOperator
participant SegWriter as DICOMSegmentationWriterOperator
participant SRWriter as DICOMTextSRWriterOperator
participant SCWriter as DICOMSCWriterOperator
participant PDF as DICOMEncapsulatedPDFWriterOperator
User->>App: Submit DICOM Study
App->>Selector: select series (rules JSON)
Selector-->>App: StudySelectedSeries
App->>Converter: convert series -> Image (DHW)
Converter-->>App: image
App->>Inference: run segmentation -> MetaTensor
Inference-->>App: seg_metatensor
App->>Metrics: compute metrics (seg_metatensor + image)
Metrics-->>App: metrics_dict
App->>ZScore: compute percentiles/z-scores (metrics_dict)
ZScore-->>App: zscore_dict + pdf_bytes
App->>Contour: generate contour (seg_metatensor)
Contour-->>App: contour image
App->>Overlay: generate overlay (seg_metatensor + image)
Overlay-->>App: overlay image
App->>SegWriter: write DICOM SEG (seg_metatensor)
SegWriter-->>App: SEG file(s)
App->>SRWriter: write DICOM SR (zscore_dict)
SRWriter-->>App: SR file
App->>PDF: write encapsulated PDF (pdf_bytes)
PDF-->>App: PDF file
App->>SCWriter: write DICOM SC (overlay image)
SCWriter-->>App: SC file
App-->>User: Deliver DICOM outputs + PDF
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
requirements-examples.txt (1)
2-13:⚠️ Potential issue | 🟠 MajorAdd
scipydependency for the z-score pipeline.The new
segmentation_zscore_operator.pydirectly imports scipy (from scipy) and usesscipy.stats.norm.ppf, butscipyis not declared in requirements-examples.txt. A clean installation of the examples will fail unless scipy is explicitly added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements-examples.txt` around lines 2 - 13, Add scipy to the example requirements so the segmentation_zscore_operator.py import (from scipy and use of scipy.stats.norm.ppf) installs correctly; update requirements-examples.txt by appending a reasonable minimum version entry like "scipy>=1.10.0" (or match your project's supported scipy baseline) to ensure the z-score pipeline can be installed and run.
🧹 Nitpick comments (12)
examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py (1)
95-97: Instance field names shadow class constants.
default_window_width,default_window_center, anddefault_voi_lut_functionshadow the class-levelDEFAULT_*constants. Consider using different names like_window_center,_window_width,_voi_lut_functionto avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py` around lines 95 - 97, The instance attributes default_window_width, default_window_center, and default_voi_lut_function shadow the class constants DEFAULT_WINDOW_WIDTH, DEFAULT_WINDOW_CENTER, and DEFAULT_VOI_LUT_FUNCTION in SegmentationOverlayOperator; rename the instance attributes to non-conflicting names (e.g., _window_width, _window_center, _voi_lut_function) and update all uses in the class (constructors, methods) to reference the new instance names so the class constants remain distinct and unambiguous.examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py (3)
295-298: Empty docstring.The
process_imagesmethod has an empty docstring. Either add documentation or remove the empty string.♻️ Proposed fix
def process_images( self, image: Union[Image, Path], study_selected_series_list: List[StudySelectedSeries], output_dir: Path ): - """ """ + """Process segmentation image and write DICOM SEG to output directory. + + Args: + image: Segmentation image as Image object, numpy array, or file path. + study_selected_series_list: List of selected DICOM series for reference. + output_dir: Output directory for the DICOM SEG file. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py` around lines 295 - 298, The method process_images(self, image: Union[Image, Path], study_selected_series_list: List[StudySelectedSeries], output_dir: Path) currently contains an empty docstring—either replace the empty triple-quote with a concise description of the method’s responsibility, parameters and return behavior (mention image, study_selected_series_list, output_dir and any side-effects), or remove the empty string entirely so no placeholder docstring remains; update the docstring text directly in the process_images definition to reflect the actual behavior implemented in that method.
181-181: Mutable default value for class attribute.
SUPPORTED_EXTENSIONSis a mutable list. While not modified in this code, it's safer to use a tuple for immutable class constants.♻️ Proposed fix
- SUPPORTED_EXTENSIONS = [".nii", ".nii.gz", ".mhd"] + SUPPORTED_EXTENSIONS = (".nii", ".nii.gz", ".mhd")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py` at line 181, SUPPORTED_EXTENSIONS is defined as a mutable list constant; change it to an immutable tuple to prevent accidental mutation by replacing the list literal with a tuple literal for the class-level constant SUPPORTED_EXTENSIONS in dicom_seg_writer_operator.py (i.e., update the SUPPORTED_EXTENSIONS symbol to use (" .nii", ".nii.gz", ".mhd") as a tuple) so the constant is immutable and safer as a class attribute.
284-287: Log the exception instead of silently swallowing it.The
try-except-passpattern hides potential issues when receiving optional input. At minimum, log at debug level.♻️ Proposed fix
try: output_folder = op_input.receive(self.input_name_output_folder) - except Exception: - pass + except Exception as e: + logging.debug(f"Optional output_folder input not provided: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py` around lines 284 - 287, Replace the silent except-pass when calling op_input.receive(self.input_name_output_folder) so the exception is logged at debug level; catch Exception as e and log it (e.g., logger.debug("Failed to receive optional output folder '%s'", self.input_name_output_folder, exc_info=True) or use self.logger.debug if the class provides one), or at minimum logger.debug with the exception message; keep the input optional behavior (do not re-raise) and consider narrowing the exception type if a specific failure is expected.examples/apps/cchmc_ped_abd_ct_seg_app/scripts/model_run.sh (1)
21-21: Document the expected working directory for this script.The relative path
../cchmc_ped_abd_ct_seg_appassumes the script is executed from thescripts/directory. Consider using$(dirname "$0")for more robust path resolution or document the expected usage.♻️ Proposed fix using script-relative paths
+# Get the directory where this script is located +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +APP_DIR="$(dirname "$SCRIPT_DIR")" + # load in environment variables source .env # remove the output directory rm -rf "$HOLOSCAN_OUTPUT_PATH" # execute model bundle locally (pythonically) -python3 ../cchmc_ped_abd_ct_seg_app -i "$HOLOSCAN_INPUT_PATH" -o "$HOLOSCAN_OUTPUT_PATH" -m "$HOLOSCAN_MODEL_PATH" +python3 "$APP_DIR" -i "$HOLOSCAN_INPUT_PATH" -o "$HOLOSCAN_OUTPUT_PATH" -m "$HOLOSCAN_MODEL_PATH"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/scripts/model_run.sh` at line 21, The script calls the app with a relative path ("../cchmc_ped_abd_ct_seg_app") which only works when run from the scripts/ directory; update the script to resolve the app path relative to the script location using script_dir="$(dirname "$0")" (or similar) and invoke "${script_dir}/../cchmc_ped_abd_ct_seg_app" (or document that the script must be executed from scripts/) so the invocation in model_run.sh reliably finds cchmc_ped_abd_ct_seg_app regardless of the current working directory.examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_selector_operator.py (3)
305-308: Log the exception when attribute lookup fails at instance level.The bare exception is caught but only logs a generic message. Consider logging the exception type for better debugging.
♻️ Proposed fix
- except Exception: - logging.info(f" Attribute {key} not at instance level either") + except Exception as ex: + logging.debug(f" Attribute {key} not at instance level either: {type(ex).__name__}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_selector_operator.py` around lines 305 - 308, The except block that handles failing to read an instance-level attribute currently swallows the exception and only logs a generic message; change the except Exception to except Exception as e and update the logging call that follows the failed lookup (the logging.info after series_attr.update) to include the exception type and message (e.g., include type(e).__name__ and e or pass exc_info) so failures when trying to read instance attributes are recorded for debugging while preserving the existing flow that continues when an attribute is not present.
197-197: Uselogging.warninginstead of deprecatedlogging.warn.
logging.warnis deprecated since Python 3.11. Uselogging.warninginstead. This also applies to lines 316, 494, 496, and 498.♻️ Proposed fix
- logging.warn("No selection rules given; select all series.") + logging.warning("No selection rules given; select all series.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_selector_operator.py` at line 197, Replace all uses of the deprecated logging.warn with logging.warning in this module: locate each call site that uses the symbol logging.warn and change it to logging.warning (ensure consistency across the module and update any unit/tests that assert warning logs if present).
200-200: Address or remove the TODO comment.The TODO comment
# TODO type is not json now.indicates incomplete work. Clarify the intended behavior or remove if no longer applicable.Would you like help addressing this TODO or creating an issue to track it?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_selector_operator.py` at line 200, The TODO comment on the line assigning selections from selection_rules is stale—either clarify the expected type or remove it: update the code around the selection_rules.get("selections", None) assignment to include a precise type hint (e.g., selections: Optional[List[Dict[str, Any]]] or the actual structure) and/or replace the TODO with a short explanatory comment stating the current expected type and where it is validated; if no follow-up is needed, just remove the TODO. Ensure you adjust any imports (typing) and add a brief unit-test or create an issue if the type needs further work.examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.py (3)
94-98: Extract nested conditional for readability.The nested ternary expression is difficult to read. Consider extracting to a helper or using explicit if-else.
♻️ Proposed refactor
- metadata = ( - segmentation_mask.meta - if isinstance(segmentation_mask, MetaTensor) - else segmentation_mask.metadata() if isinstance(segmentation_mask, Image) else None - ) + if isinstance(segmentation_mask, MetaTensor): + metadata = segmentation_mask.meta + elif isinstance(segmentation_mask, Image): + metadata = segmentation_mask.metadata() + else: + metadata = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.py` around lines 94 - 98, The nested ternary that assigns metadata is hard to read; replace it with a small helper or an explicit if/elif/else block that checks types and returns the metadata—e.g., implement a helper function get_metadata_from_segmentation_mask(segmentation_mask) that returns segmentation_mask.meta if isinstance(segmentation_mask, MetaTensor), segmentation_mask.metadata() if isinstance(segmentation_mask, Image), else None, then call metadata = get_metadata_from_segmentation_mask(segmentation_mask) where the original expression was used; reference the existing symbols segmentation_mask, MetaTensor, Image, metadata(), and metadata variable to locate and replace the code.
118-121: Log the exception instead of silently ignoring it.The exception is caught but the variable
eis unused. At minimum, log the failure reason to aid debugging when MetaTensor conversion fails.♻️ Proposed fix
try: label_image = xp.asarray(_meta) # Direct conversion to cupy array if possible except Exception as e: + self._logger.debug(f"Direct array conversion failed, falling back to CPU: {e}") label_image = _meta.cpu().numpy() # Fallback to CPU numpy array - if MetaTensor is on CPU🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.py` around lines 118 - 121, The except block swallowing the conversion error should log the caught exception before falling back; update the try/except around xp.asarray(_meta) (where label_image is assigned) to log the exception (e.g., with logging.exception or the module's logger) including the exception object `e` and a short context message like "failed to convert MetaTensor to cupy via xp.asarray", then proceed to the existing fallback label_image = _meta.cpu().numpy(); keep the same variables and behavior otherwise.
93-93: Consider avoiding deep copy for large volumes.
copy.deepcopy(segmentation_mask)can be expensive for large 3D/4D medical imaging volumes. If the original mask doesn't need to be preserved, consider operating on a view or shallow copy where safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.py` at line 93, The code creates label_image via copy.deepcopy(segmentation_mask), which is costly for large volumes; if the original segmentation_mask does not need to be preserved, operate directly on segmentation_mask (modify in place) or use a lightweight view (segmentation_mask.view()) or a faster numpy copy (segmentation_mask.copy()) instead of copy.deepcopy; update the assignment to remove copy.deepcopy and choose one of these alternatives in the segmentation_contour_operator.py where label_image and segmentation_mask are used.examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt (1)
45-47: Unpinned NVIDIA packages may cause reproducibility issues.
nvidia-nvjpeg-cu12andnvidia-nvtiff-cu12have no version constraints. This can lead to non-reproducible builds if breaking changes are introduced.♻️ Proposed fix to pin versions
nvidia-nvimgcodec-cu12>=0.6.1 -nvidia-nvjpeg-cu12 # not listed by app=sdk; may be absent based on build base image +nvidia-nvjpeg-cu12>=12.3.0 # pin to known compatible version nvidia-nvjpeg2k-cu12>=0.9.1 -nvidia-nvtiff-cu12 # nvTIFF wheel +nvidia-nvtiff-cu12>=0.4.0 # pin to known compatible version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt` around lines 45 - 47, The two NVIDIA wheels in requirements.txt are unpinned and can break reproducibility; update the entries for nvidia-nvjpeg-cu12 and nvidia-nvtiff-cu12 to use explicit version constraints (e.g., pin to a known-good release or a compatible range) instead of leaving them bare, matching your chosen policy used for nvidia-nvjpeg2k-cu12; edit the lines for nvidia-nvjpeg-cu12 and nvidia-nvtiff-cu12 to include the version specifiers and then verify by reinstalling the environment or running the CI build to confirm compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/app.py`:
- Around line 162-167: The EquipmentInfo instance currently hard-codes
software_version_number="3.5.0"; replace that with a dynamic SDK version by
importing get_sdk_semver from monai.deploy.utils.version and setting
software_version_number=get_sdk_semver() in the EquipmentInfo call (refer to the
EquipmentInfo constructor and the get_sdk_semver function used in the other
DICOM writer operators for the exact symbol names).
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.py`:
- Around line 214-239: The RGB axis detection in the 4D/3D handling block is
ambiguous; change the logic in the image_numpy handling to check the trailing
channel axis first (i.e., prefer shape[-1] == 3 for (Slices, Height, Width, 3)
or (Height, Width, 3)) and only if that is not true fall back to channel-first
layouts (shape[0] == 3 for (Channels, Slices, Height, Width) or (3, Height,
Width)). Concretely, inside the ndim == 4 branch use image_numpy.shape[3] == 3
as the primary check to determine if it is already (Slices, Height, Width,
Channels) and only then handle transposes for channel-first cases (if shape[0]
== 3 then transpose (1,0,2,3)), and update the error condition to raise only
when neither trailing nor leading channel axes indicate 3 channels; do the
analogous trailing-axis-first check for the ndim == 3 single-frame branch (check
shape[-1] == 3 before assuming shape[0] == 3) so you don’t misinterpret 3-slice
volumes as RGB.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_selector_operator.py`:
- Line 1: Update the copyright header in dicom_series_selector_operator.py from
"2021-2025" to "2021-2026" to match the other new files in the PR; locate the
top-of-file copyright comment (in the module dicom_series_selector_operator.py)
and replace the year range accordingly.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.py`:
- Around line 391-413: The code calls _create_content_sequence(result_text)
before resolving dicom_series/source_modality, so modality-aware normalization
(e.g., HU units) is skipped; move the block that receives op_input and selects
the first source series (the loop populating dicom_series from
StudySelectedSeries.selected_series) to run before building the content
sequence, compute source_modality via _get_source_modality(dicom_series) and
then call _create_content_sequence(result_text, source_modality) (and update
_create_content_sequence signature to accept and use source_modality, allowing
None when no series is available).
- Around line 405-410: The loop currently breaks only out of the inner loop, so
later StudySelectedSeries entries can overwrite dicom_series; change the logic
in dicom_text_sr_writer_operator.py where you iterate
study_selected_series_list/selected_series (the StudySelectedSeries handling) so
that once you assign dicom_series from the first selected_series you stop
iterating the outer loop as well (for example set dicom_series and then break
the outer loop or return/exit the function immediately), ensuring only the very
first selected series is used to populate the SR and no later
StudySelectedSeries entries can overwrite it.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/README.md`:
- Line 46: Fix the typos in the README workflow docs: in the output visibility
sentence referencing labels_dict and app.py remove the duplicated word "and" so
it reads "...only Liver and Spleen are included in the SR and Encapsulated PDF
outputs," and correct the misspelling "programmtically" in the series-selection
section to "programmatically"; review nearby user-facing sentences for similar
small typos and update them accordingly.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt`:
- Line 20: The requirements pin currently locks pydicom to 3.0.1 which has a
path traversal vulnerability; update the pydicom requirement in requirements.txt
(the pydicom entry) to require version 3.0.2 or newer (e.g., change the
pydicom==3.0.1 line to pydicom>=3.0.2) so installations pull the fixed release.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/scripts/map_build.sh`:
- Around line 29-30: The monai-deploy packaging command in map_build.sh uses
unquoted variables which can cause word-splitting; update the line invoking
monai-deploy package so each variable is quoted (e.g., "$HOLOSCAN_MODEL_PATH",
"${tag_prefix}:${image_version}", "${monai_deploy_app_sdk_version}",
"${cuda_version}") to ensure arguments with spaces or special chars are passed
correctly and reliably to the monai-deploy package invocation.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_metrics_operator.py`:
- Around line 249-269: The code currently auto-transposes seg_array to match
scan_array purely based on mismatched shapes (using xp.transpose and is_3d),
which can silently produce spatially misaligned masks; change this by removing
automatic shape-based transposition and instead detect shape mismatch between
seg_array and scan_array (the variables seg_array, scan_array, is_3d,
xp.transpose are involved) and fail-fast or require an explicit alignment step:
log a clear error via self._logger.error and raise an exception when shapes
differ (or require a caller-provided orientation/affine mapping parameter), and
only perform any transposition when a validated alignment/metadata check
confirms it is safe; keep the GPU movement branch (use_gpu, has_cupy,
xp.asarray) but do not attempt to auto-correct mismatched spatial
alignment—either reject, ask for explicit alignment, or use reliable spatial
metadata before transforming, and update the _logger.warning to an error
explaining that automatic transpose was disabled.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py`:
- Around line 835-959: The test() function incorrectly instantiates and calls
SegmentationMetricsOperator (references at operator_cpu, operator_gpu,
operator_gpu_fast and calls to calculate_metrics); replace those with
SegmentationOverlayOperator and rewrite the test input creation and invocation
to supply the expected arguments for SegmentationOverlayOperator (e.g., use
segmentation_mask, input_scan, and study_selected_series_list objects instead of
seg_image/scan_image and label_dict), update timing/metric checks to match the
overlay operator's API (calls/method names on SegmentationOverlayOperator) and
remove/replace any compute_components/use_gpu logic that belongs to the metrics
operator so the test exercises SegmentationOverlayOperator end-to-end.
- Line 641: Replace the deprecated cm.get_cmap call used to build jet_colormap:
change the expression jet_colormap = cm.get_cmap("jet", 256) to use the new API
jet_colormap = matplotlib.colormaps['jet'].resampled(256) and add an import for
matplotlib (e.g., import matplotlib) at the top of
segmentation_overlay_operator.py; update any references to the jet_colormap
variable as needed to ensure compatibility with the resampled colormap object.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py`:
- Around line 96-119: The optional DICOM-series input (input_name_dcm_series
declared with ConditionType.NONE in setup) is currently treated as required in
compute by immediately iterating study_selected_series_list and raising
ValueError; either make the input required by removing ConditionType.NONE in
setup for input_name_dcm_series, or update compute to handle a missing/empty
study_selected_series_list: check if
op_input.receive(self.input_name_dcm_series) is None or empty before iterating,
and when absent branch to use explicit demographic inputs (add e.g. age/sex
inputs to setup and read them in compute) or skip DICOM-dependent processing
instead of raising ValueError (adjust downstream logic that uses dicom_series
accordingly).
- Around line 350-373: The code incorrectly treats biomarker_value == 0 as "no
segmentation"; update the guard in segmentation_zscore_operator.py to only skip
when the biomarker is missing (None) or when explicit segmentation-presence
indicators in metrics (e.g., "pixel.count", "num.slices", "pixel_count", or
whatever keys your pipeline emits) indicate zero/absent segmentation; do not
treat a numeric 0.0 biomarker as absent. Locate the block using biomarker_value,
metrics, units_dict, zscore_dict, sr_name, and organ_name and replace the
condition biomarker_value is None or biomarker_value == 0 with a check that (a)
biomarker_value is None OR (b) segmentation presence keys in metrics are present
and indicate zero/absence; fall back to only None if no presence keys exist so
legitimate 0.0 values proceed to percentile/z-score calculation.
- Around line 126-130: The current PatientAge parsing only accepts values ending
with "Y" and must be extended to support DICOM ages in days/weeks/months/years;
update the code that reads patient_age_str to trim whitespace, handle units
case-insensitively, parse the numeric portion, and convert to a float in years
(e.g., D -> /365, W -> *7/365, M -> /12, Y -> as-is). Use the existing variable
names patient_age_str and patient_age and raise ValueError with the original
patient_age_str for any missing/invalid formats (non-numeric or unknown unit).
---
Outside diff comments:
In `@requirements-examples.txt`:
- Around line 2-13: Add scipy to the example requirements so the
segmentation_zscore_operator.py import (from scipy and use of
scipy.stats.norm.ppf) installs correctly; update requirements-examples.txt by
appending a reasonable minimum version entry like "scipy>=1.10.0" (or match your
project's supported scipy baseline) to ensure the z-score pipeline can be
installed and run.
---
Nitpick comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py`:
- Around line 295-298: The method process_images(self, image: Union[Image,
Path], study_selected_series_list: List[StudySelectedSeries], output_dir: Path)
currently contains an empty docstring—either replace the empty triple-quote with
a concise description of the method’s responsibility, parameters and return
behavior (mention image, study_selected_series_list, output_dir and any
side-effects), or remove the empty string entirely so no placeholder docstring
remains; update the docstring text directly in the process_images definition to
reflect the actual behavior implemented in that method.
- Line 181: SUPPORTED_EXTENSIONS is defined as a mutable list constant; change
it to an immutable tuple to prevent accidental mutation by replacing the list
literal with a tuple literal for the class-level constant SUPPORTED_EXTENSIONS
in dicom_seg_writer_operator.py (i.e., update the SUPPORTED_EXTENSIONS symbol to
use (" .nii", ".nii.gz", ".mhd") as a tuple) so the constant is immutable and
safer as a class attribute.
- Around line 284-287: Replace the silent except-pass when calling
op_input.receive(self.input_name_output_folder) so the exception is logged at
debug level; catch Exception as e and log it (e.g., logger.debug("Failed to
receive optional output folder '%s'", self.input_name_output_folder,
exc_info=True) or use self.logger.debug if the class provides one), or at
minimum logger.debug with the exception message; keep the input optional
behavior (do not re-raise) and consider narrowing the exception type if a
specific failure is expected.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_selector_operator.py`:
- Around line 305-308: The except block that handles failing to read an
instance-level attribute currently swallows the exception and only logs a
generic message; change the except Exception to except Exception as e and update
the logging call that follows the failed lookup (the logging.info after
series_attr.update) to include the exception type and message (e.g., include
type(e).__name__ and e or pass exc_info) so failures when trying to read
instance attributes are recorded for debugging while preserving the existing
flow that continues when an attribute is not present.
- Line 197: Replace all uses of the deprecated logging.warn with logging.warning
in this module: locate each call site that uses the symbol logging.warn and
change it to logging.warning (ensure consistency across the module and update
any unit/tests that assert warning logs if present).
- Line 200: The TODO comment on the line assigning selections from
selection_rules is stale—either clarify the expected type or remove it: update
the code around the selection_rules.get("selections", None) assignment to
include a precise type hint (e.g., selections: Optional[List[Dict[str, Any]]] or
the actual structure) and/or replace the TODO with a short explanatory comment
stating the current expected type and where it is validated; if no follow-up is
needed, just remove the TODO. Ensure you adjust any imports (typing) and add a
brief unit-test or create an issue if the type needs further work.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt`:
- Around line 45-47: The two NVIDIA wheels in requirements.txt are unpinned and
can break reproducibility; update the entries for nvidia-nvjpeg-cu12 and
nvidia-nvtiff-cu12 to use explicit version constraints (e.g., pin to a
known-good release or a compatible range) instead of leaving them bare, matching
your chosen policy used for nvidia-nvjpeg2k-cu12; edit the lines for
nvidia-nvjpeg-cu12 and nvidia-nvtiff-cu12 to include the version specifiers and
then verify by reinstalling the environment or running the CI build to confirm
compatibility.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/scripts/model_run.sh`:
- Line 21: The script calls the app with a relative path
("../cchmc_ped_abd_ct_seg_app") which only works when run from the scripts/
directory; update the script to resolve the app path relative to the script
location using script_dir="$(dirname "$0")" (or similar) and invoke
"${script_dir}/../cchmc_ped_abd_ct_seg_app" (or document that the script must be
executed from scripts/) so the invocation in model_run.sh reliably finds
cchmc_ped_abd_ct_seg_app regardless of the current working directory.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.py`:
- Around line 94-98: The nested ternary that assigns metadata is hard to read;
replace it with a small helper or an explicit if/elif/else block that checks
types and returns the metadata—e.g., implement a helper function
get_metadata_from_segmentation_mask(segmentation_mask) that returns
segmentation_mask.meta if isinstance(segmentation_mask, MetaTensor),
segmentation_mask.metadata() if isinstance(segmentation_mask, Image), else None,
then call metadata = get_metadata_from_segmentation_mask(segmentation_mask)
where the original expression was used; reference the existing symbols
segmentation_mask, MetaTensor, Image, metadata(), and metadata variable to
locate and replace the code.
- Around line 118-121: The except block swallowing the conversion error should
log the caught exception before falling back; update the try/except around
xp.asarray(_meta) (where label_image is assigned) to log the exception (e.g.,
with logging.exception or the module's logger) including the exception object
`e` and a short context message like "failed to convert MetaTensor to cupy via
xp.asarray", then proceed to the existing fallback label_image =
_meta.cpu().numpy(); keep the same variables and behavior otherwise.
- Line 93: The code creates label_image via copy.deepcopy(segmentation_mask),
which is costly for large volumes; if the original segmentation_mask does not
need to be preserved, operate directly on segmentation_mask (modify in place) or
use a lightweight view (segmentation_mask.view()) or a faster numpy copy
(segmentation_mask.copy()) instead of copy.deepcopy; update the assignment to
remove copy.deepcopy and choose one of these alternatives in the
segmentation_contour_operator.py where label_image and segmentation_mask are
used.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py`:
- Around line 95-97: The instance attributes default_window_width,
default_window_center, and default_voi_lut_function shadow the class constants
DEFAULT_WINDOW_WIDTH, DEFAULT_WINDOW_CENTER, and DEFAULT_VOI_LUT_FUNCTION in
SegmentationOverlayOperator; rename the instance attributes to non-conflicting
names (e.g., _window_width, _window_center, _voi_lut_function) and update all
uses in the class (constructors, methods) to reference the new instance names so
the class constants remain distinct and unambiguous.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 861c8f61-218f-4078-bc5c-7116501d2cd6
⛔ Files ignored due to path filters (12)
examples/apps/cchmc_ped_abd_ct_seg_app/assets/liver/outlier_df.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver/results_df.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver/results_f_fine.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver/results_m_fine.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver_hu/outlier_df.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver_hu/results_df.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver_hu/results_f_fine.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver_hu/results_m_fine.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/spleen/outlier_df.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/spleen/results_df.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/spleen/results_f_fine.csvis excluded by!**/*.csvexamples/apps/cchmc_ped_abd_ct_seg_app/assets/spleen/results_m_fine.csvis excluded by!**/*.csv
📒 Files selected for processing (25)
examples/apps/cchmc_ped_abd_ct_seg_app/README.mdexamples/apps/cchmc_ped_abd_ct_seg_app/abdomen_seg_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/app.pyexamples/apps/cchmc_ped_abd_ct_seg_app/app.yamlexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver/figure.htmlexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver/stats.jsonexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver_hu/figure.htmlexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver_hu/stats.jsonexamples/apps/cchmc_ped_abd_ct_seg_app/assets/spleen/figure.htmlexamples/apps/cchmc_ped_abd_ct_seg_app/assets/spleen/stats.jsonexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_selector_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/mongodb_entry_creator_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/mongodb_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/post_transforms.pyexamples/apps/cchmc_ped_abd_ct_seg_app/requirements.txtexamples/apps/cchmc_ped_abd_ct_seg_app/scripts/map_build.shexamples/apps/cchmc_ped_abd_ct_seg_app/scripts/model_run.shexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_metrics_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.pyrequirements-examples.txt
💤 Files with no reviewable changes (3)
- examples/apps/cchmc_ped_abd_ct_seg_app/mongodb_entry_creator_operator.py
- examples/apps/cchmc_ped_abd_ct_seg_app/post_transforms.py
- examples/apps/cchmc_ped_abd_ct_seg_app/mongodb_writer_operator.py
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.py (1)
161-166:⚠️ Potential issue | 🟠 MajorCheck for empty
selected_seriesbefore indexing.This path assumes
study_selected_series.selected_series[0]exists. An empty selection will raiseIndexErrorand fail with a non-actionable error.🐛 Proposed fix
for study_selected_series in study_selected_series_list: if not isinstance(study_selected_series, StudySelectedSeries): raise ValueError(f"Element in input is not expected type, {StudySelectedSeries}.") - selected_series = study_selected_series.selected_series[0] + if not study_selected_series.selected_series: + continue + selected_series = study_selected_series.selected_series[0] dicom_series = selected_series.series break + if dicom_series is None: + raise ValueError("No selected DICOM series available for metadata copy.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.py` around lines 161 - 166, The loop over study_selected_series_list assumes study_selected_series.selected_series has at least one element; before accessing selected_series[0] in the block that references StudySelectedSeries, check that study_selected_series.selected_series is truthy/non-empty and raise a clear ValueError (or continue) if it is empty; update the code around the loop that assigns selected_series and dicom_series to validate selected_series length and include contextual info (e.g., study identifier or StudySelectedSeries) in the error message so an empty selection doesn't cause an IndexError.
♻️ Duplicate comments (4)
examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py (1)
835-960:⚠️ Potential issue | 🔴 Critical
test()still targetsSegmentationMetricsOperatorand is broken.This test harness references an undefined class and methods from the metrics operator API, so running this module directly will fail.
#!/bin/bash # Read-only verification: confirm stale symbol/method usage in this file rg -n -C2 'SegmentationMetricsOperator|calculate_metrics\(' examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py` around lines 835 - 960, The test() function uses the undefined symbols SegmentationMetricsOperator and calculate_metrics (and assumes has_cupy exists), so running the module fails; fix by editing test(): locate the actual operator class defined in this module (replace SegmentationMetricsOperator with the real class name used in this file) and call the correct metrics method (replace calculate_metrics with the operator’s actual method name, e.g., compute_metrics/run/execute as defined on that class), or explicitly import the correct operator from its module; also ensure has_cupy is defined or use a safe try/except import around cupy checks before referencing it.examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py (2)
364-371:⚠️ Potential issue | 🟠 MajorUse explicit key presence when selecting
biomarker_value.The new
no_segmentationguard is better, but theorchain above it still converts a legitimate0.0intoNone. Derived metrics coming throughadditional_metrics_mapcan still be skipped before percentile calculation.🛠️ Suggested fix
- biomarker_value = ( - metrics.get("volume_ml") - or metrics.get("volume") - or metrics.get("area_cm2") - or metrics.get("area") - or metrics.get("biomarker_value") - ) + biomarker_value = None + for key in ("volume_ml", "volume", "area_cm2", "area", "biomarker_value"): + if key in metrics: + biomarker_value = metrics[key] + breakAlso applies to: 383-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py` around lines 364 - 371, The current biomarker selection uses an or-chain that treats 0.0 as falsy and will skip legitimate zero values; update the selection in the biomarker_value assignment (and the duplicated block around lines 383-395) to check key presence/None explicitly instead of relying on truthiness — e.g., test "if 'volume_ml' in metrics and metrics['volume_ml'] is not None" (and similarly for 'volume', 'area_cm2', 'area', 'biomarker_value'), or use metrics.get(key) and compare against None, so 0.0 is preserved for downstream percentile calculation.
100-118:⚠️ Potential issue | 🟠 MajorThe optional DICOM-series input is still effectively required.
setup()declaresstudy_selected_series_listoptional, butcompute()hard-fails when it is absent. That leaves callers with no usable no-DICOM path. Either make the input required, or add explicit demographic inputs and only use DICOM when present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py` around lines 100 - 118, The code currently marks the DICOM series input (self.input_name_dcm_series) optional in setup but compute() raises ValueError if it's missing, effectively making it required; update the implementation to be consistent: either (A) make the input required by changing its spec in setup, or (B) keep it optional and stop raising the error in compute — instead accept study_selected_series_list being None and use explicit demographic inputs (e.g., add PatientAge/PatientSex inputs in setup) or fallback to op_input.receive(self.input_name_metrics) for demographics; modify the compute() logic around the op_input.receive(self.input_name_dcm_series) and the subsequent ValueError to implement the chosen path so compute no longer hard-fails when the DICOM input is absent.examples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.py (1)
401-420:⚠️ Potential issue | 🟠 MajorResolve
source_modalityindependently ofcopy_tags.
dicom_seriesis still only populated inside thecopy_tagsbranch, so_create_content_sequence(..., source_modality)seesNonewhenevercopy_tags=False. In that path, CT intensity metrics lose their HU unit handling even ifstudy_selected_series_listwas provided.🛠️ Suggested fix
- dicom_series = None # It can be None if not to copy_tags. - if self.copy_tags: - # Get the first DICOM Series to retrieve study level tags. - if not study_selected_series_list or len(study_selected_series_list) < 1: - raise ValueError("Missing input, list of 'StudySelectedSeries'.") - for study_selected_series in study_selected_series_list: - if not isinstance(study_selected_series, StudySelectedSeries): - raise ValueError("Element in input is not expected type, 'StudySelectedSeries'.") - for selected_series in study_selected_series.selected_series: - dicom_series = selected_series.series - break - if dicom_series is not None: - break + dicom_series = None + if study_selected_series_list: + for study_selected_series in study_selected_series_list: + if not isinstance(study_selected_series, StudySelectedSeries): + raise ValueError("Element in input is not expected type, 'StudySelectedSeries'.") + for selected_series in study_selected_series.selected_series: + dicom_series = selected_series.series + break + if dicom_series is not None: + break + + if self.copy_tags and dicom_series is None: + raise ValueError("Missing input, list of 'StudySelectedSeries'.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.py` around lines 401 - 420, The code only sets dicom_series inside the copy_tags branch so _get_source_modality(dicom_series) can get None when copy_tags is False; change the logic so dicom_series (or at least the first available series) is extracted from study_selected_series_list regardless of copy_tags: reuse the loop that scans study_selected_series_list/StudySelectedSeries to assign dicom_series before calling _get_source_modality, or if dicom_series is still None call _get_source_modality with the first selected_series.series found; update references in the copy_tags branch to avoid duplicating the scan and ensure _create_content_sequence(result_text, source_modality) gets a resolved modality.
🧹 Nitpick comments (1)
examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py (1)
183-186: MakeSUPPORTED_EXTENSIONSimmutable.This is a shared class constant, so a tuple avoids accidental mutation across instances.
Suggested fix
- SUPPORTED_EXTENSIONS = [".nii", ".nii.gz", ".mhd"] + SUPPORTED_EXTENSIONS = (".nii", ".nii.gz", ".mhd")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py` around lines 183 - 186, The SUPPORTED_EXTENSIONS list is a mutable class constant; change it to an immutable tuple to prevent accidental mutation (replace SUPPORTED_EXTENSIONS = [".nii", ".nii.gz", ".mhd"] with a tuple form). Keep DCM_EXTENSION as-is and ensure any code that expects list methods still works (convert to list at use sites if necessary) so references to SUPPORTED_EXTENSIONS throughout dicom_seg_writer_operator.py continue to function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.py`:
- Around line 243-250: The current conversion to np.uint8 for image_numpy can
wrap out-of-range values; before any astype(np.uint8) call in the RGB conversion
block (the code that checks image_numpy.dtype != np.uint8), clip pixel values to
the 0–255 range (and if you scaled from 0–1 first, ensure scaling happens before
clipping) using numpy.clip on image_numpy so negatives and >255 values are
bounded, then cast to np.uint8; update the branch handling image_numpy.max() <=
1.0 to multiply by 255, clip to [0,255], then astype(np.uint8), and in the else
branch clip to [0,255] before astype(np.uint8).
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py`:
- Around line 287-296: The code swallows all exceptions from op_input.receive
and fails when a string output_folder reaches mkdir; change the try/except to
only handle the expected "missing input" case (do not use a bare except
Exception), re-raise any other errors, then normalize output_folder to a Path
before calling mkdir: call op_input.receive(self.input_name_output_folder)
inside a try that only handles the known missing-input indicator (e.g., None or
the specific receive exception), check if output_folder is truthy and if it's a
str convert it via Path(output_folder), otherwise fall back to
self.output_folder (also ensured to be a Path), then call
output_folder.mkdir(...) and proceed to self.process_images(seg_image,
study_selected_series_list, output_folder).
- Around line 128-130: The constructor currently allows a caller to pass
tracking_uid without tracking_id which violates the documented constraint; add a
validation in the constructor (the __init__ method in
dicom_seg_writer_operator.py) that if tracking_uid is not None and tracking_id
is None then raise a ValueError (or similar) rejecting the input, otherwise keep
the existing logic that generates a new UID using hd.UID() when tracking_id is
set and tracking_uid is None and then assign to self._tracking_uid.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/README.md`:
- Around line 75-88: The fenced assets/ tree block in README.md is missing a
language tag so markdownlint flags it; update the triple-backtick fence for the
assets directory tree (the block showing liver/, liver_hu/, spleen/ and their
files) to include a language identifier (e.g., use ```text) so the code fence
parser recognizes it—leave the contents unchanged, only add the language tag on
the opening ``` for that assets/ tree block.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/scripts/map_build.sh`:
- Around line 29-37: The script relies on relative paths and sourcing .env from
the caller's cwd; make it robust by resolving the script directory
(map_build.sh) at runtime and use that base to source the .env and construct all
paths passed to the monai-deploy package invocation (e.g., the
"../cchmc_ped_abd_ct_seg_app" project dir, the app.yaml path, and any
HOLOSCAN_MODEL_PATH references), so replace relative references with versions
built from the script's directory variable and source "$DIR/.env" before
invoking monai-deploy package to ensure paths work regardless of the current
working directory.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.py`:
- Around line 197-200: The unguarded np.squeeze(out_ndarray, 0) can raise if
axis 0 is not size 1; update the logic around the channel squeeze in
segmentation_contour_operator.py so you only remove the channel dimension when
it is actually a singleton: check out_ndarray.ndim and out_ndarray.shape[0] (or
equivalent) before calling squeeze/ indexing, fall back to leaving the array
as-is when axis-0 > 1, and then proceed to the DHW transpose
(out_ndarray.T.astype(np.uint8)) so contour generation won’t crash on
non-singleton channel-first inputs.
- Around line 103-123: The code neglects plain torch.Tensor (including CUDA)
inputs before NumPy/CuPy processing in create_contour; add an early branch
checking isinstance(label_image, torch.Tensor) and convert via label_image =
label_image.detach().cpu().numpy() (or .numpy() for CPU tensors) before
proceeding with the existing Image/MetaTensor logic, then apply the same
transpose/expand_dims handling; preserve the existing MetaTensor try/fallback
(xp.asarray/_meta.cpu().numpy()) and ensure xp is still set to np for
LabelToContour compatibility.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py`:
- Around line 305-364: The code uses a single is_on_gpu flag which causes mixed
CPU/GPU inputs to skip converting one of mask_data or scan_data before calling
_create_overlay_cupy; change to track per-input GPU state (e.g., mask_on_gpu and
scan_on_gpu) when handling segmentation_mask and scan (update the branches that
set is_on_gpu), then in the CuPy path convert each input individually only if
its corresponding flag is False so both mask_data and scan_data passed to
_create_overlay_cupy are CuPy arrays when use_cupy is True.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py`:
- Around line 427-435: The report currently hard-codes "Volume (mL)" and "Organ
Volume Quantile Curves" for every organ; instead add biomarker label and units
into the processed_organs dict (alongside "asset_name", "biomarker_value",
"percentile", "z_score", "df_m", "df_f") — e.g., add "biomarker_label" and
"biomarker_units" when populating processed_organs in
segmentation_zscore_operator.py, then update the PDF rendering code (the blocks
that build titles/axis labels around the quantile curve panels, currently
emitting "Volume (mL)" and "Organ Volume Quantile Curves") to use those new keys
so displays read "{biomarker_label} ({biomarker_units})" and "{biomarker_label}
Quantile Curves" for each organ (also update the analogous rendering at the
later section referenced in the comment, lines ~546-568).
---
Outside diff comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.py`:
- Around line 161-166: The loop over study_selected_series_list assumes
study_selected_series.selected_series has at least one element; before accessing
selected_series[0] in the block that references StudySelectedSeries, check that
study_selected_series.selected_series is truthy/non-empty and raise a clear
ValueError (or continue) if it is empty; update the code around the loop that
assigns selected_series and dicom_series to validate selected_series length and
include contextual info (e.g., study identifier or StudySelectedSeries) in the
error message so an empty selection doesn't cause an IndexError.
---
Duplicate comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.py`:
- Around line 401-420: The code only sets dicom_series inside the copy_tags
branch so _get_source_modality(dicom_series) can get None when copy_tags is
False; change the logic so dicom_series (or at least the first available series)
is extracted from study_selected_series_list regardless of copy_tags: reuse the
loop that scans study_selected_series_list/StudySelectedSeries to assign
dicom_series before calling _get_source_modality, or if dicom_series is still
None call _get_source_modality with the first selected_series.series found;
update references in the copy_tags branch to avoid duplicating the scan and
ensure _create_content_sequence(result_text, source_modality) gets a resolved
modality.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py`:
- Around line 835-960: The test() function uses the undefined symbols
SegmentationMetricsOperator and calculate_metrics (and assumes has_cupy exists),
so running the module fails; fix by editing test(): locate the actual operator
class defined in this module (replace SegmentationMetricsOperator with the real
class name used in this file) and call the correct metrics method (replace
calculate_metrics with the operator’s actual method name, e.g.,
compute_metrics/run/execute as defined on that class), or explicitly import the
correct operator from its module; also ensure has_cupy is defined or use a safe
try/except import around cupy checks before referencing it.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py`:
- Around line 364-371: The current biomarker selection uses an or-chain that
treats 0.0 as falsy and will skip legitimate zero values; update the selection
in the biomarker_value assignment (and the duplicated block around lines
383-395) to check key presence/None explicitly instead of relying on truthiness
— e.g., test "if 'volume_ml' in metrics and metrics['volume_ml'] is not None"
(and similarly for 'volume', 'area_cm2', 'area', 'biomarker_value'), or use
metrics.get(key) and compare against None, so 0.0 is preserved for downstream
percentile calculation.
- Around line 100-118: The code currently marks the DICOM series input
(self.input_name_dcm_series) optional in setup but compute() raises ValueError
if it's missing, effectively making it required; update the implementation to be
consistent: either (A) make the input required by changing its spec in setup, or
(B) keep it optional and stop raising the error in compute — instead accept
study_selected_series_list being None and use explicit demographic inputs (e.g.,
add PatientAge/PatientSex inputs in setup) or fallback to
op_input.receive(self.input_name_metrics) for demographics; modify the compute()
logic around the op_input.receive(self.input_name_dcm_series) and the subsequent
ValueError to implement the chosen path so compute no longer hard-fails when the
DICOM input is absent.
---
Nitpick comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.py`:
- Around line 183-186: The SUPPORTED_EXTENSIONS list is a mutable class
constant; change it to an immutable tuple to prevent accidental mutation
(replace SUPPORTED_EXTENSIONS = [".nii", ".nii.gz", ".mhd"] with a tuple form).
Keep DCM_EXTENSION as-is and ensure any code that expects list methods still
works (convert to list at use sites if necessary) so references to
SUPPORTED_EXTENSIONS throughout dicom_seg_writer_operator.py continue to
function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56d13195-1f71-44f9-a33a-cdb4accd6517
📒 Files selected for processing (13)
examples/apps/cchmc_ped_abd_ct_seg_app/README.mdexamples/apps/cchmc_ped_abd_ct_seg_app/app.pyexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver/figure.htmlexamples/apps/cchmc_ped_abd_ct_seg_app/assets/liver_hu/figure.htmlexamples/apps/cchmc_ped_abd_ct_seg_app/assets/spleen/figure.htmlexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_seg_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/scripts/map_build.shexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_metrics_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/apps/cchmc_ped_abd_ct_seg_app/app.py
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_metrics_operator.py (1)
247-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't auto-transpose masks based on shape coincidence alone.
This still tries
(2, 1, 0)purely because the shapes differ and keeps it when the result happens to matchscan_array.shape. That can turn an unaligned mask into a shape-compatible one and silently corrupt the metrics that feed the z-score and SR outputs. Fail fast here unless orientation has already been validated upstream from spatial metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_metrics_operator.py` around lines 247 - 257, The code in segmentation_metrics_operator.py currently auto-transposes seg_array to match scan_array when shapes differ, which can silently corrupt masks; remove the automatic transpose logic (the block using cupy.transpose/np.transpose and the transposed.shape check) and instead fail fast: if seg_array.ndim == 3 and seg_array.shape != scan_array.shape and no explicit upstream orientation validation flag is set (introduce or check a boolean like orientation_validated), raise a ValueError (or log.error and raise) explaining that segmentation orientation must be validated upstream; keep the info log only for cases where orientation_validated is True and an explicit, intentional transpose is performed.examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py (1)
100-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the DICOM-series input required or support the missing-input path.
setup()advertisesstudy_selected_series_listas optional, butcompute()immediately fails when it isn't connected. That makes the operator contract misleading for callers and turns an optional port into a guaranteed runtime error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py` around lines 100 - 118, The operator declares the DICOM-series port optional (spec.input(self.input_name_dcm_series).condition(ConditionType.NONE)) but compute() in method compute immediately raises if study_selected_series_list is missing; update the contract by either making the port required (change the spec.input call to use the required condition) or implement a missing-input path inside compute() that handles a None/empty study_selected_series_list (e.g., skip DICOM-dependent processing, use defaults from metrics_dict, or raise a clearer, documented error) — reference symbols: spec.input(self.input_name_dcm_series), input_name_dcm_series, compute, and study_selected_series_list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/scripts/map_build.sh`:
- Around line 26-33: The .env is sourced while the shell's PWD remains the
caller's directory, so variables like HOLOSCAN_MODEL_PATH that use ${PWD}
resolve incorrectly; update the script (map_build.sh) to change directory to the
app root before sourcing or to source with PWD set to that directory (e.g.,
perform cd "${script_dir}/.." prior to source "${script_dir}/../.env" or
otherwise normalize HOLOSCAN_MODEL_PATH after sourcing) so that variables
referenced in .env (HOLOSCAN_MODEL_PATH) are expanded relative to the app's
directory rather than the caller's current working directory.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.py`:
- Around line 595-605: The current code normalizes label colors by
max(label_volume) (variables label_volume, label_normalized, label_uint8, val),
causing label colors to shift depending on which labels are present; replace
this dynamic normalization with a fixed label→RGB lookup table so each integer
label ID always maps to the same RGB. Concretely, create a constant LUT mapping
label IDs to RGB (for CPU use a numpy array and for GPU use a cupy array) and
replace the normalization/colormap steps (the GPU path around
label_volume→label_uint8→val and the CPU path referenced at lines 629-640) with
an indexed lookup (e.g., lut[label_volume]) or cupy.take so the output is
directly an RGB image of shape (H,W,3) with deterministic colors per label.
Ensure the LUT covers all expected label IDs and keep dtype conversions
consistent (uint8) for downstream code.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py`:
- Around line 315-323: The interpolation branch in
segmentation_zscore_operator.py where sorted_biomarkers, sorted_quantiles and
biomarker_value are used can divide by zero when b_high == b_low; modify the
inner loop that computes percentile so that before performing (biomarker_value -
b_low) / (b_high - b_low) you check if b_high - b_low == 0 and in that case set
percentile to a safe value (e.g., q_low or (q_low+q_high)/2) instead of
dividing; keep the existing break after assigning percentile so the loop exits
as before.
---
Duplicate comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_metrics_operator.py`:
- Around line 247-257: The code in segmentation_metrics_operator.py currently
auto-transposes seg_array to match scan_array when shapes differ, which can
silently corrupt masks; remove the automatic transpose logic (the block using
cupy.transpose/np.transpose and the transposed.shape check) and instead fail
fast: if seg_array.ndim == 3 and seg_array.shape != scan_array.shape and no
explicit upstream orientation validation flag is set (introduce or check a
boolean like orientation_validated), raise a ValueError (or log.error and raise)
explaining that segmentation orientation must be validated upstream; keep the
info log only for cases where orientation_validated is True and an explicit,
intentional transpose is performed.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py`:
- Around line 100-118: The operator declares the DICOM-series port optional
(spec.input(self.input_name_dcm_series).condition(ConditionType.NONE)) but
compute() in method compute immediately raises if study_selected_series_list is
missing; update the contract by either making the port required (change the
spec.input call to use the required condition) or implement a missing-input path
inside compute() that handles a None/empty study_selected_series_list (e.g.,
skip DICOM-dependent processing, use defaults from metrics_dict, or raise a
clearer, documented error) — reference symbols:
spec.input(self.input_name_dcm_series), input_name_dcm_series, compute, and
study_selected_series_list.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 390b4c7c-5e0b-45b3-b408-9e33b6cc261a
📒 Files selected for processing (7)
examples/apps/cchmc_ped_abd_ct_seg_app/README.mdexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_sc_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/scripts/map_build.shexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_contour_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_metrics_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_overlay_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
examples/apps/cchmc_ped_abd_ct_seg_app/decoder_nvimgcodec.py (3)
366-369: 💤 Low valueMerge adjacent f-strings into a single f-string.
Same pattern as above—two adjacent f-strings should be consolidated.
🔧 Suggested fix
- _logger.debug( - f"Using UNCHANGED color spec to preserve YCbCr " f"(as_rgb=False, PI: {photometric_interpretation})" - ) + _logger.debug( + f"Using UNCHANGED color spec to preserve YCbCr (as_rgb=False, PI: {photometric_interpretation})" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/apps/cchmc_ped_abd_ct_seg_app/decoder_nvimgcodec.py` around lines 366 - 369, The debug call in decoder_nvimgcodec.py uses two adjacent f-strings in the _logger.debug call; replace them with a single f-string in the _logger.debug invocation that includes the full message and the photometric_interpretation variable (reference symbols: _logger.debug, photometric_interpretation, the "Using UNCHANGED color spec..." message) so the string is concatenated properly in one f-string.Source: Linters/SAST tools
555-557: 💤 Low valueUse
min()instead of sorting to find the lexicographically smallest candidate.
sorted(candidates)[0]creates a fully sorted list just to retrieve the minimum element. Usingmin(candidates)is clearer and more efficient.🔧 Suggested fix
if candidates: - # deterministic fallback - return sorted(candidates)[0] + # deterministic fallback - return lexicographically smallest + return min(candidates)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/apps/cchmc_ped_abd_ct_seg_app/decoder_nvimgcodec.py` around lines 555 - 557, Replace the inefficient sorted(candidates)[0] fallback with min(candidates) to obtain the lexicographically smallest element without fully sorting; locate the branch that checks the candidates list (the if candidates: block in decoder_nvimgcodec.py) and change the return expression to use min(candidates) so it is clearer and more performant.Source: Linters/SAST tools
345-347: 💤 Low valueMerge adjacent f-strings into a single f-string.
Two adjacent f-strings are implicitly concatenated. While this works, it's clearer as a single f-string.
🔧 Suggested fix
- _logger.debug( - f"Using RGB color spec for JPEG 2000 color transformation " f"(PI: {photometric_interpretation})" - ) + _logger.debug( + f"Using RGB color spec for JPEG 2000 color transformation (PI: {photometric_interpretation})" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/apps/cchmc_ped_abd_ct_seg_app/decoder_nvimgcodec.py` around lines 345 - 347, Merge the two adjacent f-strings passed to _logger.debug into a single f-string so the message is clearer; locate the debug call using _logger.debug and photometric_interpretation in decoder_nvimgcodec.py and replace the concatenated f-strings with one f-string containing the full message including "(PI: {photometric_interpretation})".Source: Linters/SAST tools
examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.py (4)
286-289: 💤 Low valueFix typo: "poisition" should be "position".
The variable names
image_poisition_patient_deandimage_poisition_patienthave a typo. While this doesn't affect functionality, it reduces code readability.🔧 Suggested fix
- image_poisition_patient_de = slice[0x0020, 0x0032] - if image_poisition_patient_de is not None: - image_poisition_patient = image_poisition_patient_de.value - slice_position = image_poisition_patient + image_position_patient_de = sop_instance[0x0020, 0x0032] + if image_position_patient_de is not None: + image_position_patient = image_position_patient_de.value + slice_position = image_position_patient🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.py` around lines 286 - 289, Variables image_poisition_patient_de and image_poisition_patient contain a typo; rename them to image_position_patient_de and image_position_patient (and update all their usages, including the slice lookup slice[0x0020, 0x0032] and the assignment to slice_position) so the variable names read "position" consistently for readability.
48-49: 💤 Low valueConsider marking these dicts as
ClassVarfor clarity.While the current usage (
metadata.update(self.METADATA_SPACE_RAS)) doesn't mutate these class-level dicts, Ruff flags them as mutable class attributes. Adding aClassVartype hint documents intent and satisfies the linter.🔧 Suggested fix
+from typing import ClassVar, Dict, List, Union class DICOMSeriesToVolumeOperator(Operator): ... MONAI_TRANSFORMS_SPATIAL_METADATA_NAME = "space" - METADATA_SPACE_RAS = {MONAI_TRANSFORMS_SPATIAL_METADATA_NAME: MONAI_UTIL_ENUMS_SPACEKEYS_RAS} - METADATA_SPACE_LPS = {MONAI_TRANSFORMS_SPATIAL_METADATA_NAME: MONAI_UTIL_ENUMS_SPACEKEYS_LPS} + METADATA_SPACE_RAS: ClassVar[Dict[str, str]] = {MONAI_TRANSFORMS_SPATIAL_METADATA_NAME: MONAI_UTIL_ENUMS_SPACEKEYS_RAS} + METADATA_SPACE_LPS: ClassVar[Dict[str, str]] = {MONAI_TRANSFORMS_SPATIAL_METADATA_NAME: MONAI_UTIL_ENUMS_SPACEKEYS_LPS}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.py` around lines 48 - 49, Annotate the two class-level dictionaries METADATA_SPACE_RAS and METADATA_SPACE_LPS as ClassVar to indicate they are immutable class attributes for the linter: add "from typing import ClassVar" to imports and change their annotations to something like METADATA_SPACE_RAS: ClassVar[dict[str, str]] and METADATA_SPACE_LPS: ClassVar[dict[str, str]] (or ClassVar[Mapping[str, str]] if preferred) while leaving their current values and usage (e.g., metadata.update(self.METADATA_SPACE_RAS)) unchanged.Source: Linters/SAST tools
177-179: ⚡ Quick winUse
logging.exception()to capture the full traceback.
logging.error(f"...: {e}")only logs the exception message. Usinglogging.exception()within an except block automatically includes the full traceback, which is more useful for debugging.🔧 Suggested fix
except Exception as e: - logging.error(f"Failed to apply rescale to DICOM volume: {e}") + logging.exception("Failed to apply rescale to DICOM volume") raise RuntimeError("Failed to apply rescale to DICOM volume.") from eApply the same pattern to lines 221 and 233.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.py` around lines 177 - 179, Replace the plain logging.error calls inside the except blocks with logging.exception so the full traceback is captured: in the except that currently logs "Failed to apply rescale to DICOM volume" (and the two other except blocks at the later handlers around lines 221 and 233 in dicom_series_to_volume_operator.py), change logging.error(f"...: {e}") to logging.exception("Failed to apply rescale to DICOM volume") (or the corresponding descriptive message for those blocks) and keep the existing raise RuntimeError(... ) from e so the exception is re-raised with the original context.Source: Linters/SAST tools
270-270: 💤 Low valueRename
sliceto avoid shadowing the Python builtin.Using
sliceas a variable name shadows the built-inslicetype. Consider renaming todicom_sliceorsop_instance.🔧 Suggested fix
- for slice_index, slice in enumerate(series._sop_instances): + for slice_index, sop_instance in enumerate(series._sop_instances): ... - image_orientation_patient_de = slice[0x0020, 0x0037] + image_orientation_patient_de = sop_instance[0x0020, 0x0037] ... - image_poisition_patient_de = slice[0x0020, 0x0032] + image_position_patient_de = sop_instance[0x0020, 0x0032]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.py` at line 270, The loop variable `slice` in the for loop "for slice_index, slice in enumerate(series._sop_instances):" shadows the Python builtin; rename it (e.g., `sop_instance` or `dicom_slice`) and update every use inside that loop and any nested scopes (references to `slice`) to the new name to avoid builtin shadowing and potential bugs.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.py`:
- Around line 110-114: The loop over study_selected_series_list can raise
IndexError because it assumes study_selected_series.selected_series has at least
one element; update the loop in dicom_series_to_volume_operator.py to guard
before accessing selected_series[0] by checking that
study_selected_series.selected_series is truthy/non-empty (or has len>0) and
either skip or raise a clear ValueError when empty; reference the
StudySelectedSeries type check and the variables study_selected_series,
selected_series and dicom_series so you modify the correct block to handle empty
selections safely.
- Around line 397-414: The code assumes ImagePositionPatient was found and
directly indexes ip1/ipn, which can be None if (0020,0032) is missing; update
the block around s_1/s_n handling (the try/except and variables ip1, ipn) to
explicitly check after the try whether ip1 or ipn is None (or missing) and
either raise a clear ValueError describing which slice (s_1 or s_n) lacks
ImagePositionPatient or attempt a safe fallback (e.g., use Dataset.get/attribute
access with a default or derive position from other tags) before indexing;
ensure the error message names ip1/ipn and the originating dataset (s_1/s_n) so
callers can diagnose the missing tag.
- Around line 366-377: The code reads the ImageOrientationPatient tag into
image_orientation_patient but does not guard if it's missing or empty, causing a
TypeError when indexing; after the try/except that sets
image_orientation_patient (from s_1[0x0020,0x0037]), add a check that
image_orientation_patient is not None and has at least 6 elements (or raise a
clear ValueError/RuntimeError) before accessing indices 0..5, including a
descriptive message identifying the dataset/series (e.g., reference s_1 or its
SeriesInstanceUID) so callers know which DICOM is malformed.
In `@examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt`:
- Around line 2-4: Remove the global --trusted-host directives from
requirements.txt (the lines containing “--trusted-host pypi.org”,
“--trusted-host pypi.python.org”, and “--trusted-host files.pythonhosted.org”);
instead, revert the file to only list package pins and move any temporary
--trusted-host usage into deployment or local setup docs/scripts as an explicit
override and/or add a short README note explaining how to use that override when
necessary.
- Around line 1-4: The requirements.txt currently contains only --trusted-host
directives so pip installs no packages; restore the app's actual Python
dependencies by re-adding the package lines (e.g., the project-specific
packages, pinned versions, or an include of another requirements file) above or
below the existing --trusted-host entries so they remain supplemental; ensure
the file contains the required package names used by the app (imports referenced
by the service) and keep the three --trusted-host lines intact for environments
needing them.
---
Nitpick comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/decoder_nvimgcodec.py`:
- Around line 366-369: The debug call in decoder_nvimgcodec.py uses two adjacent
f-strings in the _logger.debug call; replace them with a single f-string in the
_logger.debug invocation that includes the full message and the
photometric_interpretation variable (reference symbols: _logger.debug,
photometric_interpretation, the "Using UNCHANGED color spec..." message) so the
string is concatenated properly in one f-string.
- Around line 555-557: Replace the inefficient sorted(candidates)[0] fallback
with min(candidates) to obtain the lexicographically smallest element without
fully sorting; locate the branch that checks the candidates list (the if
candidates: block in decoder_nvimgcodec.py) and change the return expression to
use min(candidates) so it is clearer and more performant.
- Around line 345-347: Merge the two adjacent f-strings passed to _logger.debug
into a single f-string so the message is clearer; locate the debug call using
_logger.debug and photometric_interpretation in decoder_nvimgcodec.py and
replace the concatenated f-strings with one f-string containing the full message
including "(PI: {photometric_interpretation})".
In `@examples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.py`:
- Around line 286-289: Variables image_poisition_patient_de and
image_poisition_patient contain a typo; rename them to image_position_patient_de
and image_position_patient (and update all their usages, including the slice
lookup slice[0x0020, 0x0032] and the assignment to slice_position) so the
variable names read "position" consistently for readability.
- Around line 48-49: Annotate the two class-level dictionaries
METADATA_SPACE_RAS and METADATA_SPACE_LPS as ClassVar to indicate they are
immutable class attributes for the linter: add "from typing import ClassVar" to
imports and change their annotations to something like METADATA_SPACE_RAS:
ClassVar[dict[str, str]] and METADATA_SPACE_LPS: ClassVar[dict[str, str]] (or
ClassVar[Mapping[str, str]] if preferred) while leaving their current values and
usage (e.g., metadata.update(self.METADATA_SPACE_RAS)) unchanged.
- Around line 177-179: Replace the plain logging.error calls inside the except
blocks with logging.exception so the full traceback is captured: in the except
that currently logs "Failed to apply rescale to DICOM volume" (and the two other
except blocks at the later handlers around lines 221 and 233 in
dicom_series_to_volume_operator.py), change logging.error(f"...: {e}") to
logging.exception("Failed to apply rescale to DICOM volume") (or the
corresponding descriptive message for those blocks) and keep the existing raise
RuntimeError(... ) from e so the exception is re-raised with the original
context.
- Line 270: The loop variable `slice` in the for loop "for slice_index, slice in
enumerate(series._sop_instances):" shadows the Python builtin; rename it (e.g.,
`sop_instance` or `dicom_slice`) and update every use inside that loop and any
nested scopes (references to `slice`) to the new name to avoid builtin shadowing
and potential bugs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71f721e2-2992-431a-9770-248491243064
📒 Files selected for processing (9)
examples/apps/cchmc_ped_abd_ct_seg_app/README.mdexamples/apps/cchmc_ped_abd_ct_seg_app/__main__.pyexamples/apps/cchmc_ped_abd_ct_seg_app/app.pyexamples/apps/cchmc_ped_abd_ct_seg_app/app.yamlexamples/apps/cchmc_ped_abd_ct_seg_app/decoder_nvimgcodec.pyexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_series_to_volume_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.pyexamples/apps/cchmc_ped_abd_ct_seg_app/requirements.txtexamples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py
✅ Files skipped from review due to trivial changes (2)
- examples/apps/cchmc_ped_abd_ct_seg_app/main.py
- examples/apps/cchmc_ped_abd_ct_seg_app/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/apps/cchmc_ped_abd_ct_seg_app/app.yaml
- examples/apps/cchmc_ped_abd_ct_seg_app/segmentation_zscore_operator.py
- examples/apps/cchmc_ped_abd_ct_seg_app/app.py
- examples/apps/cchmc_ped_abd_ct_seg_app/dicom_text_sr_writer_operator.py
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt`:
- Line 10: Update the MONAI pin in requirements for the app: replace the hard
pin "monai==1.5.1" in examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt
with a minimum-safe constraint like "monai>=1.5.2" to mitigate known
RCE/Zip-Slip issues; after updating, run the app's tests and CI, and search the
codebase for usages that download NGC private bundles or call pickle-related
ingestion (e.g., any code that opens .zip from remote sources or uses
pickle.load) to assess and remediate any remaining risk.
- Line 29: The dependency entry currently allows Pillow versions vulnerable to
critical CVEs; update the requirements.txt line that references "Pillow>=8.0.0"
to require a safe minimum (e.g., "Pillow>=10.0.0") so pip will install a patched
release; ensure any related packaging metadata or dependency pins in the repo
that also reference Pillow are aligned to the new minimum.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 802794c2-6359-4bd2-a478-a6981be1c3ec
📒 Files selected for processing (1)
examples/apps/cchmc_ped_abd_ct_seg_app/requirements.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated the CCHMC CT Abdominal Segmentation Example App:
For Z-score analysis, computed organ metrics are compared against sex-stratified, age-specific normative reference data to produce clinically interpretable z-scores and percentiles.
See
README.mdfor more detailed description.Summary by CodeRabbit
New Features
Updates
Documentation
Removals