ENH: Provide TotalSegmentatorWithContrast as a custom seg option#77
Merged
Conversation
WalkthroughThe PR moves contrast handling into ChangesContrast-aware segmentation refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors “contrast-enhanced” handling for TotalSegmentator by removing the contrast_enhanced_study plumbing from workflows/segmenters and instead routing “contrast mode” through a dedicated SegmentChestTotalSegmentatorWithContrast implementation, then updates CLIs, tutorials, experiments, tests, and docs to use the new selection pattern.
Changes:
- Removes
contrast_enhanced_studysupport fromWorkflowConvertImageToVTK.run_workflow()and fromSegmentChestTotalSegmentator(including dropping the"contrast"taxonomy group there). - Updates CLI factories to choose a contrast-capable segmenter via
build_segmentation_method(..., contrast=...)rather than mutating a plain segmenter instance. - Updates tests/tutorials/docs/experiments to call
segmenter.segment(image)(no contrast flag) and to useSegmentChestTotalSegmentatorWithContrastwhere a"contrast"mask is required.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_02_ct_to_vtk.py | Switches tutorial to the contrast-capable segmenter and removes contrast_enhanced_study workflow arg. |
| tests/test_workflow_fit_statistical_model_to_patient.py | Updates expectations after removing contrast_enhanced_study passthrough. |
| tests/test_workflow_convert_image_to_usd.py | Updates default segmenter assertions to the contrast-capable variant and removes contrast-flag expectations. |
| tests/test_segment_chest_total_segmentator.py | Splits contrast expectations: plain segmenter has no "contrast" key; contrast segmenter does. |
| tests/conftest.py | Adds a session fixture for SegmentChestTotalSegmentatorWithContrast. |
| src/physiomotion4d/workflow_fit_statistical_model_to_patient.py | Removes contrast_enhanced_study=False when invoking image→VTK workflow. |
| src/physiomotion4d/workflow_convert_image_to_vtk.py | Removes contrast_enhanced_study parameter and related segmenter mutation logic. |
| src/physiomotion4d/workflow_convert_image_to_usd.py | Switches default segmentation backend to the contrast-capable variant. |
| src/physiomotion4d/segment_chest_total_segmentator.py | Removes contrast-specific state/methods and adds a subclass hook for extra taxonomy groups. |
| src/physiomotion4d/cli/convert_image_to_vtk.py | Routes --contrast into the segmentation-method factory rather than workflow args. |
| src/physiomotion4d/cli/convert_image_to_usd.py | Routes --contrast into the segmentation-method factory; removes old TotalSegmentator-specific mutation. |
| src/physiomotion4d/cli/_method_factories.py | Adds contrast parameter to segmentation factory and selects the contrast-capable backend when requested. |
| src/physiomotion4d/init.py | Exposes SegmentChestTotalSegmentatorWithContrast from the public package API. |
| README.md | Updates examples to use the contrast-capable segmenter rather than a runtime contrast flag. |
| experiments/Lung-GatedCT_To_USD/Experiment_SegReg.py | Removes contrast flag from segmenter call sites. |
| experiments/Heart-VTKSeries_To_USD/1-heart_vtkseries_to_usd.py | Switches to the contrast-capable segmenter and removes contrast flag usage. |
| experiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.py | Removes contrast flag usage and related contrast-mask handling in the example. |
| experiments/Heart-GatedCT-OptimizedLongitudinalRegistration/3-eval_icon.py | Removes contrast flag from segmenter call sites. |
| experiments/Heart-GatedCT_To_USD/2-generate_segmentation.py | Switches to the contrast-capable segmenter and removes contrast-flag mutation calls. |
| experiments/Heart-GatedCT_To_USD/1-register_images.py | Switches to the contrast-capable segmenter and removes contrast flag usage. |
| docs/troubleshooting.rst | Updates guidance to pick the contrast-capable segmenter for contrast-enhanced studies. |
| docs/quickstart.rst | Updates quickstart examples to remove contrast_enhanced/contrast flags and use the new segmenter. |
| docs/examples.rst | Updates examples to use the contrast-capable segmenter and removes contrast flags. |
| docs/developer/workflows.rst | Removes the outdated contrast_enhanced parameter usage in workflow examples. |
| docs/developer/segmentation.rst | Removes contrast flag usage from the segmentation contract example. |
| docs/cli_scripts/byod_tutorials.rst | Updates BYOD tutorial examples to select segmenter explicitly instead of contrast_enhanced. |
| docs/api/workflows.rst | Updates API docs examples to use the contrast-capable segmenter and removes contrast flags. |
| docs/api/segmentation/totalsegmentator.rst | Updates TotalSegmentator docs: plain segmenter has no "contrast"; contrast-capable variant adds it. |
| docs/api/segmentation/simpleware.rst | Removes contrast flag usage from Simpleware usage examples. |
| docs/api/segmentation/index.rst | Removes contrast flag usage from segmentation index examples. |
| docs/api/segmentation/base.rst | Removes contrast flag usage from the base contract examples. |
Comments suppressed due to low confidence (1)
src/physiomotion4d/workflow_convert_image_to_usd.py:115
- The PR title suggests adding
TotalSegmentatorWithContrastas an optional/custom segmentation backend, but this change also makes it the default segmenter forWorkflowConvertImageToUSDwhensegmentation_method=None. If this default swap is intended, the PR title/description should reflect that behavioral change; otherwise consider keeping the previous default and only using the contrast segmenter when explicitly requested.
if segmentation_method is None:
segmentation_method = SegmentChestTotalSegmentatorWithContrast(
log_level=log_level
)
elif not isinstance(segmentation_method, SegmentAnatomyBase):
raise TypeError(
"segmentation_method must be a SegmentAnatomyBase instance or None"
)
self.segmenter: SegmentAnatomyBase = segmentation_method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+69
| from .segment_chest_total_segmentator_with_contrast import ( | ||
| SegmentChestTotalSegmentatorWithContrast, | ||
| ) |
|
|
||
|
|
||
| def build_segmentation_method(name: str) -> SegmentAnatomyBase: | ||
| def build_segmentation_method(name: str, contrast: bool = False) -> SegmentAnatomyBase: |
Comment on lines
203
to
205
| contrast_mask_yes = result_with_contrast["contrast"] | ||
| segmenter_total_segmentator.set_contrast_enhanced_study(False) | ||
|
|
||
| # Both should return valid masks | ||
| assert contrast_mask_no is not None, "Contrast mask (no flag) is None" | ||
| assert contrast_mask_yes is not None, "Contrast mask (with flag) is None" | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/physiomotion4d/cli/convert_image_to_usd.py (1)
138-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the ICON mass-preservation toggle for non-contrast runs.
RegisterImagesICONdefaultsuse_mass_preservationtoFalse, so removingset_mass_preservation(not args.contrast)changes non-contrast CLI runs from the previousTruesetting toFalse.src/physiomotion4d/cli/convert_image_to_usd.py:138-142🤖 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 `@src/physiomotion4d/cli/convert_image_to_usd.py` around lines 138 - 142, Restore the ICON mass-preservation toggle in the RegisterImagesICON branch of convert_image_to_usd by setting use_mass_preservation based on the contrast flag before running registration. In the convert_image_to_usd CLI flow, keep the previous behavior by calling the appropriate RegisterImagesICON setter so non-contrast runs preserve mass and contrast runs do not, alongside the existing set_number_of_iterations logic.docs/api/segmentation/base.rst (1)
40-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
SegmentChestTotalSegmentatorshould not listcontrasthere. Update this example to reservecontrastforSegmentChestTotalSegmentatorWithContrast; the plain segmenter’s key set isheart, lung, bone, major_vessels, soft_tissue, other.🤖 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 `@docs/api/segmentation/base.rst` around lines 40 - 48, The example in the segmentation docs is incorrect because SegmentChestTotalSegmentator is shown with a contrast key it does not produce. Update the example in base.rst so the SegmentChestTotalSegmentator key set only includes heart, lung, bone, major_vessels, soft_tissue, and other, and reserve contrast for SegmentChestTotalSegmentatorWithContrast; keep the surrounding explanation about segmenter-specific membership unchanged.docs/quickstart.rst (1)
118-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRename
project_nametousd_project_namein both quickstart snippets.
WorkflowConvertImageToUSDtakesusd_project_name, so the examples atdocs/quickstart.rst:118-123anddocs/quickstart.rst:152-157are broken as written.🤖 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 `@docs/quickstart.rst` around lines 118 - 123, In the quickstart examples that instantiate WorkflowConvertImageToUSD, replace the incorrect project_name argument with usd_project_name so the documented call matches the actual constructor signature; update both snippets in the quickstart docs and keep the rest of the example unchanged.docs/examples.rst (1)
33-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRename
project_nametousd_project_namein theWorkflowConvertImageToUSDexamples. The constructor takesusd_project_name, so the snippets atdocs/examples.rst:33-38,409-414, and478-483will raise aTypeErroras written.🤖 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 `@docs/examples.rst` around lines 33 - 38, Update the `WorkflowConvertImageToUSD` example snippets to use `usd_project_name` instead of `project_name`, since the constructor expects that argument and the current examples will fail with a TypeError. Make the same rename in all affected `WorkflowConvertImageToUSD` usages in the docs examples so the parameter name matches the workflow API consistently.docs/developer/segmentation.rst (1)
19-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the contrast-handling prose.
docs/developer/segmentation.rst:74-76still attributes contrast-agent fusion toSegmentAnatomyBase; that behavior now belongs inSegmentChestTotalSegmentatorWithContrast.🤖 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 `@docs/developer/segmentation.rst` around lines 19 - 28, The contrast-handling prose in the segmentation developer docs is outdated and still attributes contrast-agent fusion to SegmentAnatomyBase. Update the text in docs/developer/segmentation.rst to describe that this behavior now belongs to SegmentChestTotalSegmentatorWithContrast, and make sure any nearby references to segmenter responsibilities or taxonomy-based outputs remain consistent with the current class names and key-based mask contract.
🤖 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 `@docs/api/workflows.rst`:
- Around line 57-63: The example for WorkflowConvertImageToUSD uses the wrong
constructor argument name, so update the snippet to use usd_project_name instead
of project_name. Keep the rest of the example unchanged and make sure the
parameter name matches the WorkflowConvertImageToUSD initializer so the docs
reflect the actual API.
In `@docs/cli_scripts/byod_tutorials.rst`:
- Around line 93-98: Update the WorkflowConvertImageToUSD Python API snippets to
use the correct constructor argument name: replace project_name with
usd_project_name in the calls to pm4d.WorkflowConvertImageToUSD. Make this
change consistently in both tutorial examples so the documented usage matches
the actual WorkflowConvertImageToUSD signature and avoids TypeError.
In `@docs/troubleshooting.rst`:
- Around line 72-83: The troubleshooting example uses WorkflowConvertImageToUSD
without importing it, so update the documented Python snippet to include the
workflow import alongside SegmentChestTotalSegmentatorWithContrast. Keep the
import block in the same code example and make sure the snippet is
copy/pasteable by referencing WorkflowConvertImageToUSD and
SegmentChestTotalSegmentatorWithContrast together.
---
Outside diff comments:
In `@docs/api/segmentation/base.rst`:
- Around line 40-48: The example in the segmentation docs is incorrect because
SegmentChestTotalSegmentator is shown with a contrast key it does not produce.
Update the example in base.rst so the SegmentChestTotalSegmentator key set only
includes heart, lung, bone, major_vessels, soft_tissue, and other, and reserve
contrast for SegmentChestTotalSegmentatorWithContrast; keep the surrounding
explanation about segmenter-specific membership unchanged.
In `@docs/developer/segmentation.rst`:
- Around line 19-28: The contrast-handling prose in the segmentation developer
docs is outdated and still attributes contrast-agent fusion to
SegmentAnatomyBase. Update the text in docs/developer/segmentation.rst to
describe that this behavior now belongs to
SegmentChestTotalSegmentatorWithContrast, and make sure any nearby references to
segmenter responsibilities or taxonomy-based outputs remain consistent with the
current class names and key-based mask contract.
In `@docs/examples.rst`:
- Around line 33-38: Update the `WorkflowConvertImageToUSD` example snippets to
use `usd_project_name` instead of `project_name`, since the constructor expects
that argument and the current examples will fail with a TypeError. Make the same
rename in all affected `WorkflowConvertImageToUSD` usages in the docs examples
so the parameter name matches the workflow API consistently.
In `@docs/quickstart.rst`:
- Around line 118-123: In the quickstart examples that instantiate
WorkflowConvertImageToUSD, replace the incorrect project_name argument with
usd_project_name so the documented call matches the actual constructor
signature; update both snippets in the quickstart docs and keep the rest of the
example unchanged.
In `@src/physiomotion4d/cli/convert_image_to_usd.py`:
- Around line 138-142: Restore the ICON mass-preservation toggle in the
RegisterImagesICON branch of convert_image_to_usd by setting
use_mass_preservation based on the contrast flag before running registration. In
the convert_image_to_usd CLI flow, keep the previous behavior by calling the
appropriate RegisterImagesICON setter so non-contrast runs preserve mass and
contrast runs do not, alongside the existing set_number_of_iterations logic.
🪄 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: 15c796a2-dba6-444b-9088-4fb62b897ecb
📒 Files selected for processing (31)
README.mddocs/api/segmentation/base.rstdocs/api/segmentation/index.rstdocs/api/segmentation/simpleware.rstdocs/api/segmentation/totalsegmentator.rstdocs/api/workflows.rstdocs/cli_scripts/byod_tutorials.rstdocs/developer/segmentation.rstdocs/developer/workflows.rstdocs/examples.rstdocs/quickstart.rstdocs/troubleshooting.rstexperiments/Heart-GatedCT-OptimizedLongitudinalRegistration/3-eval_icon.pyexperiments/Heart-GatedCT_To_USD/1-register_images.pyexperiments/Heart-GatedCT_To_USD/2-generate_segmentation.pyexperiments/Heart-Statistical_Model_To_Patient/heart_model_to_patient.pyexperiments/Heart-VTKSeries_To_USD/1-heart_vtkseries_to_usd.pyexperiments/Lung-GatedCT_To_USD/Experiment_SegReg.pysrc/physiomotion4d/__init__.pysrc/physiomotion4d/cli/_method_factories.pysrc/physiomotion4d/cli/convert_image_to_usd.pysrc/physiomotion4d/cli/convert_image_to_vtk.pysrc/physiomotion4d/segment_chest_total_segmentator.pysrc/physiomotion4d/workflow_convert_image_to_usd.pysrc/physiomotion4d/workflow_convert_image_to_vtk.pysrc/physiomotion4d/workflow_fit_statistical_model_to_patient.pytests/conftest.pytests/test_segment_chest_total_segmentator.pytests/test_workflow_convert_image_to_usd.pytests/test_workflow_fit_statistical_model_to_patient.pytutorials/tutorial_02_ct_to_vtk.py
💤 Files with no reviewable changes (3)
- docs/developer/workflows.rst
- tests/test_workflow_fit_statistical_model_to_patient.py
- src/physiomotion4d/workflow_fit_statistical_model_to_patient.py
Comment on lines
57
to
63
| workflow = WorkflowConvertImageToUSD( | ||
| input_filenames=["cardiac_4d.nrrd"], | ||
| contrast_enhanced=True, | ||
| output_directory="./results", | ||
| project_name="patient_001", | ||
| segmentation_method=SegmentChestTotalSegmentator(), | ||
| segmentation_method=SegmentChestTotalSegmentatorWithContrast(), | ||
| registration_method=RegisterImagesICON(), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## file list"
git ls-files docs/api/workflows.rst
git ls-files | rg '(^|/)(.*workflow.*|.*test.*|.*convert.*usd.*)\.(py|rst|md|ts|js)$' || true
echo
echo "## relevant snippets from docs/api/workflows.rst"
wc -l docs/api/workflows.rst
cat -n docs/api/workflows.rst | sed -n '45,85p'
echo
echo "## search for WorkflowConvertImageToUSD and usd_project_name"
rg -n "WorkflowConvertImageToUSD|usd_project_name|project_name" . -g '!**/.git/**' -g '!**/node_modules/**'Repository: Project-MONAI/physiomotion4d
Length of output: 14494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## docs/api/workflows.rst excerpt"
wc -l docs/api/workflows.rst
sed -n '55,70p' docs/api/workflows.rst | cat -n
echo
echo "## declarations/usages"
rg -n "class WorkflowConvertImageToUSD|def __init__|usd_project_name|project_name" . -g '!**/.git/**' -g '!**/node_modules/**'Repository: Project-MONAI/physiomotion4d
Length of output: 11051
Rename project_name to usd_project_name in this example. WorkflowConvertImageToUSD takes usd_project_name, so this snippet doesn’t match the constructor as written.
🤖 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 `@docs/api/workflows.rst` around lines 57 - 63, The example for
WorkflowConvertImageToUSD uses the wrong constructor argument name, so update
the snippet to use usd_project_name instead of project_name. Keep the rest of
the example unchanged and make sure the parameter name matches the
WorkflowConvertImageToUSD initializer so the docs reflect the actual API.
Comment on lines
93
to
98
| workflow = pm4d.WorkflowConvertImageToUSD( | ||
| input_filenames=["patient_dicom_dir"], | ||
| contrast_enhanced=False, | ||
| segmentation_method=pm4d.SegmentChestTotalSegmentator(), | ||
| output_directory="./results", | ||
| project_name="patient_heart", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the tutorial file and relevant workflow/API references.
git ls-files docs/cli_scripts/byod_tutorials.rst
rg -n "WorkflowConvertImageToUSD|usd_project_name|project_name" docs/cli_scripts/byod_tutorials.rst . -g '!**/.git/**'Repository: Project-MONAI/physiomotion4d
Length of output: 11356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tutorial sections around the referenced lines.
sed -n '80,155p' docs/cli_scripts/byod_tutorials.rstRepository: Project-MONAI/physiomotion4d
Length of output: 2354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search tests or docs that define the expected keyword for this workflow.
rg -n "usd_project_name|project_name" . -g '!**/.git/**' -g '!docs/cli_scripts/byod_tutorials.rst'Repository: Project-MONAI/physiomotion4d
Length of output: 5156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If this file is generated from examples elsewhere, inspect nearby docs to compare.
rg -n "WorkflowConvertImageToUSD" docs . -g '!**/.git/**'Repository: Project-MONAI/physiomotion4d
Length of output: 7905
Rename project_name to usd_project_name in both Python API snippets.
WorkflowConvertImageToUSD takes usd_project_name, so the calls at lines 93-98 and 140-146 will raise TypeError as written.
🤖 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 `@docs/cli_scripts/byod_tutorials.rst` around lines 93 - 98, Update the
WorkflowConvertImageToUSD Python API snippets to use the correct constructor
argument name: replace project_name with usd_project_name in the calls to
pm4d.WorkflowConvertImageToUSD. Make this change consistently in both tutorial
examples so the documented usage matches the actual WorkflowConvertImageToUSD
signature and avoids TypeError.
Comment on lines
+72
to
83
| 1. Check if image is contrast-enhanced. Use | ||
| :class:`SegmentChestTotalSegmentatorWithContrast` instead of | ||
| :class:`SegmentChestTotalSegmentator` for contrast-enhanced studies: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| from physiomotion4d import SegmentChestTotalSegmentatorWithContrast | ||
|
|
||
| workflow = WorkflowConvertImageToUSD( | ||
| ..., | ||
| contrast_enhanced=True # or False | ||
| segmentation_method=SegmentChestTotalSegmentatorWithContrast(), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the missing workflow import.
WorkflowConvertImageToUSD is used here without being imported, so the snippet is not copy/pasteable. Please add the workflow import alongside SegmentChestTotalSegmentatorWithContrast.
🤖 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 `@docs/troubleshooting.rst` around lines 72 - 83, The troubleshooting example
uses WorkflowConvertImageToUSD without importing it, so update the documented
Python snippet to include the workflow import alongside
SegmentChestTotalSegmentatorWithContrast. Keep the import block in the same code
example and make sure the snippet is copy/pasteable by referencing
WorkflowConvertImageToUSD and SegmentChestTotalSegmentatorWithContrast together.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/physiomotion4d/workflow_convert_image_to_vtk.py (2)
51-57: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring: still states default is
SegmentChestTotalSegmentator.Line 57 reads "Defaults to a new :class:
SegmentChestTotalSegmentatorwhen omitted," but the actual default (per lines 94 and 104-106) is nowSegmentChestTotalSegmentatorWithContrast. This directly contradicts the updated__init__docstring/behavior a few lines below.📝 Proposed fix
- segmentation with pulmonary/great-vessel branches trimmed). Defaults to - a new :class:`SegmentChestTotalSegmentator` when omitted. + segmentation with pulmonary/great-vessel branches trimmed). Defaults to + a new :class:`SegmentChestTotalSegmentatorWithContrast` when omitted.🤖 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 `@src/physiomotion4d/workflow_convert_image_to_vtk.py` around lines 51 - 57, Update the stale docstring for the segmentation_method parameter in workflow_convert_image_to_vtk.py so it matches the actual default used by the constructor. The current text still says the default is SegmentChestTotalSegmentator, but __init__ now uses SegmentChestTotalSegmentatorWithContrast when omitted; revise that sentence and any related wording in the docstring to reflect the current behavior, keeping the references to SegmentAnatomyBase and the concrete segmenter classes consistent.
92-106: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the plain default for
WorkflowConvertImageToVTK
WorkflowConvertImageToVTK()now instantiatesSegmentChestTotalSegmentatorWithContrast, but this workflow's docstring and tests still expectSegmentChestTotalSegmentator. That changes the implicit behavior for every caller that omitssegmentation_methodand can relabel non-contrast CTs as contrast. Make the contrast variant explicit instead, or revert the default to the plain segmenter.🤖 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 `@src/physiomotion4d/workflow_convert_image_to_vtk.py` around lines 92 - 106, The default segmentation backend in WorkflowConvertImageToVTK has been changed to the contrast variant, but the documented and tested behavior still expects the plain segmenter. Update the default handling in WorkflowConvertImageToVTK.__init__ so callers omitting segmentation_method get SegmentChestTotalSegmentator, or otherwise make the contrast choice explicit without changing the implicit default. Keep the type check and constructor flow intact, but ensure the default path matches the existing WorkflowConvertImageToVTK docstring and tests.
🧹 Nitpick comments (1)
src/physiomotion4d/segment_chest_total_segmentator_with_contrast.py (1)
180-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
preprocessed_imageto reflect raw CT inputsegment_contrast_agent()is called with the original image, and its HU thresholding depends on raw intensities. Update the parameter/docstring toinput_imageorraw_imageso normalized data isn’t passed in later and contrast detection silently breaks.🤖 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 `@src/physiomotion4d/segment_chest_total_segmentator_with_contrast.py` around lines 180 - 225, The segment_contrast_agent() parameter name and documentation are misleading because it operates on raw CT intensities, not a preprocessed image. Rename preprocessed_image to input_image or raw_image in segment_contrast_agent() and update the docstring/argument description to match, then ensure the call to segment_connected_component() uses the renamed variable consistently so normalized data is not passed in by mistake.
🤖 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 `@src/physiomotion4d/segment_chest_total_segmentator_with_contrast.py`:
- Around line 40-48: Add an explicit `-> None` return annotation to the
`__init__` method in `SegmentChestTotalSegmentatorWithContrast` so it satisfies
mypy strict mode with `disallow_untyped_defs = true`. Keep the existing
`log_level` argument typing and `super().__init__(log_level=log_level)`
initialization unchanged, and only update the constructor signature to make the
return type explicit.
- Around line 54-56: The type annotations in this module use the deprecated
lowercase ITK alias, which is invalid for strict typing. Update the
`postprocess_after_labelmap` signature and any other annotations in this file to
use `itk.Image` instead of `itk.image`, including the `input_image`,
`labelmap_image`, and return types so the class and method definitions remain
type-correct.
---
Outside diff comments:
In `@src/physiomotion4d/workflow_convert_image_to_vtk.py`:
- Around line 51-57: Update the stale docstring for the segmentation_method
parameter in workflow_convert_image_to_vtk.py so it matches the actual default
used by the constructor. The current text still says the default is
SegmentChestTotalSegmentator, but __init__ now uses
SegmentChestTotalSegmentatorWithContrast when omitted; revise that sentence and
any related wording in the docstring to reflect the current behavior, keeping
the references to SegmentAnatomyBase and the concrete segmenter classes
consistent.
- Around line 92-106: The default segmentation backend in
WorkflowConvertImageToVTK has been changed to the contrast variant, but the
documented and tested behavior still expects the plain segmenter. Update the
default handling in WorkflowConvertImageToVTK.__init__ so callers omitting
segmentation_method get SegmentChestTotalSegmentator, or otherwise make the
contrast choice explicit without changing the implicit default. Keep the type
check and constructor flow intact, but ensure the default path matches the
existing WorkflowConvertImageToVTK docstring and tests.
---
Nitpick comments:
In `@src/physiomotion4d/segment_chest_total_segmentator_with_contrast.py`:
- Around line 180-225: The segment_contrast_agent() parameter name and
documentation are misleading because it operates on raw CT intensities, not a
preprocessed image. Rename preprocessed_image to input_image or raw_image in
segment_contrast_agent() and update the docstring/argument description to match,
then ensure the call to segment_connected_component() uses the renamed variable
consistently so normalized data is not passed in by mistake.
🪄 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: 56b84375-8450-4c38-8d8d-689bfe4ccf19
📒 Files selected for processing (2)
src/physiomotion4d/segment_chest_total_segmentator_with_contrast.pysrc/physiomotion4d/workflow_convert_image_to_vtk.py
Comment on lines
+40
to
+48
| def __init__(self, log_level: int | str = logging.INFO): | ||
| """Initialize the contrast-enhanced TotalSegmentator-based segmentation. | ||
|
|
||
| Args: | ||
| log_level: Logging level (default: logging.INFO) | ||
| """ | ||
| super().__init__(log_level=log_level) | ||
|
|
||
| self.contrast_threshold: int = 500 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing -> None return annotation on __init__.
Under mypy strict mode (disallow_untyped_defs = true), __init__ must also declare its return type.
🔧 Proposed fix
- def __init__(self, log_level: int | str = logging.INFO):
+ def __init__(self, log_level: int | str = logging.INFO) -> None:As per coding guidelines, "Use full type hints with mypy strict mode (disallow_untyped_defs = true)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def __init__(self, log_level: int | str = logging.INFO): | |
| """Initialize the contrast-enhanced TotalSegmentator-based segmentation. | |
| Args: | |
| log_level: Logging level (default: logging.INFO) | |
| """ | |
| super().__init__(log_level=log_level) | |
| self.contrast_threshold: int = 500 | |
| def __init__(self, log_level: int | str = logging.INFO) -> None: | |
| """Initialize the contrast-enhanced TotalSegmentator-based segmentation. | |
| Args: | |
| log_level: Logging level (default: logging.INFO) | |
| """ | |
| super().__init__(log_level=log_level) | |
| self.contrast_threshold: int = 500 |
🤖 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 `@src/physiomotion4d/segment_chest_total_segmentator_with_contrast.py` around
lines 40 - 48, Add an explicit `-> None` return annotation to the `__init__`
method in `SegmentChestTotalSegmentatorWithContrast` so it satisfies mypy strict
mode with `disallow_untyped_defs = true`. Keep the existing `log_level` argument
typing and `super().__init__(log_level=log_level)` initialization unchanged, and
only update the constructor signature to make the return type explicit.
Source: Coding guidelines
Comment on lines
+54
to
+56
| def postprocess_after_labelmap( | ||
| self, input_image: itk.image, labelmap_image: itk.image | ||
| ) -> itk.image: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the itk Python package define itk.image as a class or only as a deprecated function alias for itk.output()?
💡 Result:
In the itk Python package, itk.Image (with a capital "I") is a class, whereas itk.image (with a lowercase "i") is a deprecated function [1][2][3]. Specifically: - itk.Image is the template class used to instantiate ITK image objects (e.g., ImageType = itk.Image[itk.UC, 2]) [2][4]. - itk.image is a legacy function that acts as an alias for itk.output [1][5]. Its usage triggers a warning stating that it is deprecated and that itk.output should be used instead [1][6].
Citations:
- 1: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Wrapping/Generators/Python/itk/support/extras.py
- 2: https://examples.itk.org/src/core/common/createanimage/documentation
- 3: itk.image() to itk.output() in _type_aliases.py InsightSoftwareConsortium/itkwidgets#742
- 4: https://itkpythonpackage.readthedocs.io/en/latest/Quick_start_guide.html
- 5: https://github.com/InsightSoftwareConsortium/ITK/blob/v5.2.1/Wrapping/Generators/Python/itk/support/extras.py
- 6: https://github.com/InsightSoftwareConsortium/ITK/blob/a8ca2578c53906cf2116413c74c27f2aa037e464/Wrapping/Generators/Python/itk/support/extras.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/physiomotion4d/segment_chest_total_segmentator_with_contrast.py"
echo "== File outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Matching annotations =="
rg -n "itk\.image|itk\.Image" "$FILE" || true
echo
echo "== Relevant file excerpt =="
wc -l "$FILE"
sed -n '1,240p' "$FILE"Repository: Project-MONAI/physiomotion4d
Length of output: 10726
Replace itk.image with itk.Image in this module.
itk.image is a deprecated function alias, not a type, so these annotations are invalid for strict type checking. Update the parameter and return annotations throughout this file.
🤖 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 `@src/physiomotion4d/segment_chest_total_segmentator_with_contrast.py` around
lines 54 - 56, The type annotations in this module use the deprecated lowercase
ITK alias, which is invalid for strict typing. Update the
`postprocess_after_labelmap` signature and any other annotations in this file to
use `itk.Image` instead of `itk.image`, including the `input_image`,
`labelmap_image`, and return types so the class and method definitions remain
type-correct.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 36.04% 36.07% +0.03%
==========================================
Files 57 58 +1
Lines 7275 7271 -4
==========================================
+ Hits 2622 2623 +1
+ Misses 4653 4648 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Summary by CodeRabbit
segment(image)signature and removed reliance on extra contrast-study flags.contrastresult).