Add enhanced DICOM support with CLI integration and tests - #846
Add enhanced DICOM support with CLI integration and tests#846Dhananjhay wants to merge 17 commits into
Conversation
yarikoptic
left a comment
There was a problem hiding this comment.
we need to potentially figure out better level where we need to introduce such a change -- I do not think that groupping code should be anyhow different
| dist/ | ||
| sample_nifti.json | ||
| venvs/ | ||
| heudiconv/_version.py |
There was a problem hiding this comment.
unrelated, not needed, should not be here
| action="store_true", | ||
| default=False, | ||
| help="Use enhanced DICOM metadata extraction module for multi-frame " | ||
| "Enhanced DICOM files. This provides better handling of Enhanced MR " |
There was a problem hiding this comment.
how better is better? should it be used only for enhanced or generally?
| get_reproducible_int([str(tmp_path)]) | ||
|
|
||
|
|
||
| def test_validate_dicom_with_use_enhanced_dicom_false() -> None: |
There was a problem hiding this comment.
we now have a marker to use in such cases, see
https://github.com/nipy/heudiconv/blob/master/tox.ini#L30
and instruct or agent to annotate added tests accordingly
| def test_validate_dicom_with_use_enhanced_dicom_false() -> None: | |
| @pytest.mark.ai_generated | |
| def test_validate_dicom_with_use_enhanced_dicom_false() -> None: |
e.g. I have
❯ grep -B2 -A4 ai_ ~/.claude/CLAUDE.md
## Testing with pytest
- When adding tests generated by AI assistants to projects that use pytest, ALWAYS mark them with `@pytest.mark.ai_generated`:
```python
@pytest.mark.ai_generated
async def test_my_new_feature() -> None:
"""Test description."""
# test code| assert len(series_id) == 2 # (series_number, protocol_name) | ||
|
|
||
|
|
||
| def test_validate_dicom_with_use_enhanced_dicom_true() -> None: |
There was a problem hiding this comment.
for such ones -- instruct your agent to create parametric tests... those AIs like to breed tests too much
|
Thanks @yarikoptic, we will actually retract this back to the drawing board as we've discovered a number of issues.. I have a better idea of enhanced dicoms now so alot of this will be re-conceptualized. Overall the issues we are trying to resolve are for certain enhanced dicoms: 1) metadata (eg seqinfo) not extracted properly, and 2) nifti conversion not working properly. I am thinking a solution to 1) could be a PR here (some pointers on how to do this in the cleanest way would be helpful!), and 2) either try to tackle in dcm2niix, or make an alternative optional converter. |
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
Co-authored-by: akhanf <11492701+akhanf@users.noreply.github.com>
cd518a4 to
e53a266
Compare
Summary
This PR adds support for Enhanced DICOM datasets, enabling robust metadata extraction and CLI-level integration for workflows that rely on modern multi-dimensional DICOM formats.
Note: This PR is intended to be merged after the sanitize-feature PR and will be rebased accordingly once that PR lands.
Key changes
Introduces an enhanced DICOM metadata extraction module
Adds --use-enhanced-dicom CLI option and threads it through the codebase
Refactors enhanced DICOM handling into independent, testable functions
Supports extraction of multi-dimensional parameters (e.g., multiple echo times)
Fixes TR/TE extraction from Enhanced DICOM functional groups
Adds comprehensive integration tests for enhanced DICOM workflows
Applies formatting and linting cleanups where relevant
Dependency on prior PR
This PR is logically independent but touches adjacent code paths affected by the sanitize-feature PR. To keep each change reviewable:
The sanitize-feature PR is submitted first
This PR will be rebased on top of it once merged
Testing
Added integration tests covering --use-enhanced-dicom
Existing test suite passes
Tests cover CLI wiring and metadata extraction paths