Open bpinsard opened 4 months ago
Hi @bpinsard -- sounds important. Any sample dicoms ? may be some among dcm2niix test ones (ping @rordenlab)
The XA dicoms I got are not mine. However, it seems that the dcm2niix XA test data show that behavior. eg. https://github.com/neurolabusc/dcm_qa_nih/blob/master/Ref/DCM2NIIX_regression_test_4.json only has PulseSequenceName and not SequenceName tag.
ah, what could go wrong, right? ;-) that sequence_name
was added by @leej3 in 2017 in 5a7fb1f869d4ee9e2a0d415551e2639d5b1b6af7 and it seems that no existing here heuristics use it:
❯ git grep '\[19\]' | grep -v '\.dcm'
❯ git grep sequence_name
heudiconv/dicoms.py: sequence_name = dcminfo[0x18, 0x24].value
heudiconv/dicoms.py: sequence_name = dcminfo[0x19, 0x109C].value
heudiconv/dicoms.py: sequence_name = ""
heudiconv/dicoms.py: sequence_name=sequence_name,
heudiconv/utils.py: sequence_name: str # 19
and no tests test it? (not good). FWIW on my Siemens sample files I do not see that field at all
The XA dicoms I got are not mine. However, it seems that the dcm2niix XA test data show that behavior. eg. https://github.com/neurolabusc/dcm_qa_nih/blob/master/Ref/DCM2NIIX_regression_test_4.json only has PulseSequenceName and not SequenceName tag.
well -- there the epiRT seems to be found only in GE data if I get naming of folders correctly:
❯ pwd
/home/yoh/deb/gits/pkg-exppsy/dcm2niix/dcm_qa_nih/In/20180918Si
❯ dcmdump */*dcm | grep epiRT | head
❯ cd -
~exppsy/dcm2niix/dcm_qa_nih/In/20180918GE
README-Study.txt* mr_0004/ mr_0005/ mr_0006/ mr_0007/
❯ dcmdump */*dcm | grep epiRT | head
(0019,109c) LO [epiRT] # 6, 1 PulseSequenceName
do you have private data which would attest to that field being useful on XA** data?
(Sorry for the super late reply, slammed with 10**10 things since I got back from vacations.)
My bad for posting the wrong repo: https://github.com/neurolabusc/dcm_qa_xa30 contains XA dicoms with similar problem: only PulseSequenceName
containing the pertinent info for seqinfo (and reflected in dcm2niix sidecars).
I try to rely as much as possible on sequence info rather than idiosyncratic ProtocolName
when I write heuristics, but yes it should be covered by tests.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 82.12%. Comparing base (
41b6d83
) to head (49c5783
). Report is 4 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Added really basic test that grabs all the dicoms in the test data folder and try to create seqinfo for each one.
It effectively covers the current fix.
As create_seqinfo
was not covered at all in unit tests, I caught and fixed another problem with ProtocolName.
@yarikoptic let me know if that's enough testing for that PR.
Thanks!
fixes #752