extract sequence_name from PulseSequenceName on Siemens XA** data

[bpinsard]

fixes #752

• need shareable test data
• write tests

[yarikoptic]

Hi @bpinsard -- sounds important. Any sample dicoms ? may be some among dcm2niix test ones (ping @rordenlab)

[bpinsard]

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.

[yarikoptic]

ah, what could go wrong, right? ;-) that sequence_name was added by @leej3 in 2017 in 5a7fb1f 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?

[bpinsard]

(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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.12%. Comparing base (41b6d83) to head (49c5783).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #753      +/-   ##
==========================================
+ Coverage   82.08%   82.12%   +0.03%     
==========================================
  Files          42       42              
  Lines        4215     4224       +9     
==========================================
+ Hits         3460     3469       +9     
  Misses        755      755              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bpinsard]

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!