DICOM Text SR Writer Updates#578
Conversation
WalkthroughThis PR corrects DICOM metadata encoding issues and enhances SR writer compliance across three operator modules. CodeMeaning strings in Contributing Equipment Sequences are fixed, SR-specific metadata fields are added, and FileMetaInformation handling is refined. Copyright years are updated. No functional control flow is altered. ChangesDICOM Writer Operators and Utilities
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
bluna301
force-pushed
the
bl/dicom_text_sr_writer_op_updates
branch
2 times, most recently
from
51063df to
a3a64b2
Compare
|
bluna301
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
requirements-min.txt (1)
3-3: Consider tracking pkg_resources migration to importlib as separate technical debt work.The codebase uses
pkg_resources.working_setinmonai/deploy/utils/importutil.py(4 functions:is_dist_editable(),dist_module_path(),is_module_installed(),dist_requires()). While the setuptools <81.0.0 pin correctly addresses the immediate deprecation issue, migrating toimportlib.metadatawould eliminate this version constraint and is a worthwhile long-term improvement. However, this refactoring is best tracked as separate work since it requires code changes beyond the scope of the requirements file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements-min.txt` at line 3, Create a technical-debt task to migrate uses of pkg_resources to importlib.metadata for the functions is_dist_editable(), dist_module_path(), is_module_installed(), and dist_requires() in monai/deploy/utils/importutil.py; the task should list the exact functions to change, describe replacing pkg_resources.working_set calls with importlib.metadata (or importlib_metadata for older Python), include tests to preserve current behavior (editable/install detection and requirement parsing), and note that once migration is complete the setuptools>=59.5.0,<81.0.0 pin in requirements-min.txt can be revisited or removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@requirements-min.txt`:
- Line 3: Create a technical-debt task to migrate uses of pkg_resources to
importlib.metadata for the functions is_dist_editable(), dist_module_path(),
is_module_installed(), and dist_requires() in monai/deploy/utils/importutil.py;
the task should list the exact functions to change, describe replacing
pkg_resources.working_set calls with importlib.metadata (or importlib_metadata
for older Python), include tests to preserve current behavior (editable/install
detection and requirement parsing), and note that once migration is complete the
setuptools>=59.5.0,<81.0.0 pin in requirements-min.txt can be revisited or
removed.
chezhia
left a comment
chezhia
left a comment
There was a problem hiding this comment.
All minor changes, if we are enabling enhanced SR support in this PR, please make a not in the relevant docs too, so people are aware we support it.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…m typo fix Signed-off-by: bluna301 <luna.bryanr@gmail.com>
MMelQin
force-pushed
the
bl/dicom_text_sr_writer_op_updates
branch
from
07b50eb to
8769f64
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/deploy/operators/dicom_utils.py (1)
201-201: ⚡ Quick winRemove the commented-out assignment.
Please delete this stale commented code line instead of keeping it inline; it leaves a lingering analyzer warning and reduces readability.
🤖 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 `@monai/deploy/operators/dicom_utils.py` at line 201, Remove the stale commented-out assignment line "# file_meta.FileMetaInformationGroupLength = 198" from monai/deploy/operators/dicom_utils.py to eliminate the lingering analyzer warning and improve readability; simply delete that single commented line where it appears near the file_meta usage in the DICOM metadata assembly code (search for the file_meta variable or surrounding DICOM metadata setup functions to locate it).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.
Nitpick comments:
In `@monai/deploy/operators/dicom_utils.py`:
- Line 201: Remove the stale commented-out assignment line "#
file_meta.FileMetaInformationGroupLength = 198" from
monai/deploy/operators/dicom_utils.py to eliminate the lingering analyzer
warning and improve readability; simply delete that single commented line where
it appears near the file_meta usage in the DICOM metadata assembly code (search
for the file_meta variable or surrounding DICOM metadata setup functions to
locate it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72f50476-29c6-4377-a1e3-2debd1515132
📒 Files selected for processing (3)
monai/deploy/operators/dicom_seg_writer_operator.pymonai/deploy/operators/dicom_text_sr_writer_operator.pymonai/deploy/operators/dicom_utils.py
✅ Files skipped from review due to trivial changes (1)
- monai/deploy/operators/dicom_seg_writer_operator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/deploy/operators/dicom_text_sr_writer_operator.py
3 participants
When preparing MAP pipelines for clinical integration, the DICOM SRs produced by the default
DICOMTextSRWriterOperatorwere validated using libraries such as dciodvfy and dicom-validator. The following validation logs are produced:The non-standard attributes that throw warnings are defined in write_common_modules within
dicom_utils.py; these are recognized in the source code as extensions of the DICOM IOD. This PR resolves the remaining warnings and errors.The
FileMetaInformationtags on the DICOM SR are written as follows (MicroDICOM viewer):This is manually set in
write_common_modules:However, this seems to encode the character
0and1to their UTF-8 byte values of0x30and0x31, respectively, which results in a value ofb"\x30\x31"-->30\31. This was updated to write the literal bytes00 01i.e.b"\x00\x01";FileMetaInformationGroupLengthsetting was also removed, as this will be handled automatically during DICOM writing.CompletionFlag indicates the degree of completeness of the DICOM SR. Considering the DICOM SR is complete in the context of the AI model (i.e. it is a finalized AI output from the model), this tag value was set to
COMPLETE.ReferencedPerformedProcedureStepSequence uniquely identifies the
Performed Procedure Step SOP Instancefor which the series is created. However, we are not tracking the Modality Performed Procedure Step (MPPS) via MAPs/MONAI Deploy; while this tag isType 3 (Optional)for most modalities, it is not copied bywrite_common_modules. As such, this tag value was written as an emptySequence.PerformedProcedureCodeSequence conveys the codes of the performed procedures pertaining to this SOP Instance. However, we are not tracking the Modality Performed Procedure Step (
MPPS) via MAPs/MONAI Deploy and this tag is not present for most imaging modalities. As such, this tag value was written as an emptySequence.Miscellaneous Updates:
seg_purpose_of_reference_code.CodeMeaning = '"Processing Algorithm'typo correctedsetuptools<81.0.0pin;pkg_resourcesis deprecated above this versiontestfilesSummary by CodeRabbit
Bug Fixes
Chores