Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhanced dicom fixes #45

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Enhanced dicom fixes #45

wants to merge 3 commits into from

Conversation

akhanf
Copy link
Member

@akhanf akhanf commented Nov 18, 2024

This tries to address the specific issues in #44, related to the CFMM 3T software upgrade.

  1. fixes parsing to accept more generic StudyID tags from the filename
  2. updates the cfmm_base heuristic to allow 'NONE' to mean the same thing as 'ND' when checking whether there is any distortion correction (e.g. DIS2D, DIS3D) applied to the image.
  3. updates to latest heudiconv and dcm2niix, as they include fixes to get enhanced dicom on Siemens data to work.

The issue is that the Bruker enhanced dicoms still require our custom fork (https://github.com/AlanKuurstra/heudiconv, branch=unstacked_dcm). @AlanKuurstra, what was the reason we opted to not make a PR into heudiconv for those changes? That would be the ideal situation, otherwise we could perhaps just update that branch with latest heudiconv changes and continue as is (perhaps that is what we do in the short-term anyhow)..

to indicate no distortion correction applied - note: should confirm this
is the case.
@akhanf akhanf mentioned this pull request Nov 18, 2024
@AlanKuurstra
Copy link
Contributor

AlanKuurstra commented Nov 18, 2024

Two issues needed to be addressed when we forked heudiconv, one regarding time zones and the other metadata.

The time zone changes have now been merged upstream.
nipy/heudiconv#780

The additional problem of filling the metadata sidecar file with 4d and 5d dicom metadata is not resolved upstream afaik. The problem needs to be addressed in one of heudiconv’s dependencies, dcmstack:
moloney/dcmstack#84
If none of our users are using the additional metadata, I would suggest using the official heudiconv repo and employing the minmeta option which will avoid the metadata bug until the changes in dcmstack have been merged.

Honestly, we may want to use minmeta by default even after upstream supports n-d enhanced dicoms. Heudiconv converts dicom to nifti using dcm2niix, but if minmeta is not used it will convert the dicom to nifti a second time with dcmstack only for the purpose of concatenating dicom metadata for the bids sidecar file, which is inefficient especially for some of our larger datasets.

If users need more metadata than is provided with the minmeta option, we can always require them to extract it themselves using their heuristics file and defining a custom_callable function as done here

@akhanf
Copy link
Member Author

akhanf commented Nov 20, 2024

I posted this in the issue, but repeating here for context

The use of --minmeta wasn't making a difference for me (ie it was still choking on the Bruker dicoms), and applying the commit you had (nipy/heudiconv@def8691) to the latest heudiconv doesn't fix things either, not sure if it is the same issue or a new one though..

alik@AFI-CBS-H-1:/local/scratch$ singularity run -e ./tar2bids_enhdicom.sif -h cfmm_bruker.py -O "--minmeta" -o test_enhdicom_bruker_updated_minmeta -P '{subject}' Everling_Marmoset_20230718_Betty_20230718_01.484D54A4.tar
	Overriding heuristic file as: cfmm_bruker.py
	Using heudiconv options: --minmeta
	Overriding output dir as: test_enhdicom_bruker_updated_minmeta
  Using custom PatientName search: {subject}
  PI=Everling Study=Marmoset Date=20230718 PatientName=Betty
session not found in {subject}
Everling_Marmoset_20230718_Betty_20230718_01.484D54A4.tar -> sub-Betty
  Running: heudiconv ... 
heudiconv --minmeta --overwrite -b -d /local/scratch/Everling_Marmoset_20230718_\{subject\}_20230718_01.484D54A4.tar -o /local/scratch/test_enhdicom_bruker_updated_minmeta -f /opt/tar2bids/heuristics/cfmm_bruker.py -s Betty  | tee /local/scratch/test_enhdicom_bruker_updated_minmeta/code/tar2bids_2024-11-20_11h13m_32370/logs/heudiconv.Betty
INFO: Running heudiconv version 1.3.2+enhdicom.0 latest 1.3.2
INFO: Need to process 1 study sessions
INFO: PROCESSING STARTS: {'subject': 'Betty', 'outdir': '/local/scratch/test_enhdicom_bruker_updated_minmeta/', 'session': None}
INFO: Processing 19 dicoms
INFO: Analyzing 19 dicoms
Traceback (most recent call last):
  File "/usr/local/bin/heudiconv", line 33, in <module>
    sys.exit(load_entry_point('heudiconv==1.3.2+enhdicom.0', 'console_scripts', 'heudiconv')())
  File "/usr/local/lib/python3.9/dist-packages/heudiconv/cli/run.py", line 30, in main
    workflow(**kwargs)
  File "/usr/local/lib/python3.9/dist-packages/heudiconv/main.py", line 479, in workflow
    prep_conversion(
  File "/usr/local/lib/python3.9/dist-packages/heudiconv/convert.py", line 217, in prep_conversion
    seqinfo = group_dicoms_into_seqinfos(
  File "/usr/local/lib/python3.9/dist-packages/heudiconv/dicoms.py", line 338, in group_dicoms_into_seqinfos
    mwinfo = validate_dicom(filename, dcmfilter)
  File "/usr/local/lib/python3.9/dist-packages/heudiconv/dicoms.py", line 175, in validate_dicom
    del mw.series_signature[sig]
  File "/usr/lib/python3.9/functools.py", line 969, in __get__
    val = self.func(instance)
  File "/usr/local/lib/python3.9/dist-packages/nibabel/nicom/dicomwrappers.py", line 886, in series_signature
    signature['image_shape'] = (self.image_shape, eq)
  File "/usr/lib/python3.9/functools.py", line 969, in __get__
    val = self.func(instance)
  File "/usr/local/lib/python3.9/dist-packages/nibabel/nicom/dicomwrappers.py", line 770, in image_shape
    raise WrapperError('Non-singular index precedes the slice index')
nibabel.nicom.dicomwrappers.WrapperError: Non-singular index precedes the slice index

I think I will just merge this PR to get the 3T functional again, then can make a new issue for the above

use heudiconv fork with unstacked dcm commit

This uses the latest heudiconv and cherry-picks the commit from AlanKuurstra:

support extended metadata from single unstacked dicom, dimension > 3
@akhanf akhanf marked this pull request as ready for review November 20, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants