ENH: Support SBRef and phase data in ReproIn heuristic #387

tsalo · 2019-10-24T00:57:31Z

Changes proposed:

Remove 'P' from automatic sequence type detection. Originally it assumed all 'P' files are field maps, but they can also be phase data.
Expand sequence type detection for functional scans to include single-band reference volumes (if the series description ends with _SBRef) and functional phase data (if the image type tuple includes one entry that is 'P').
~~Fix a couple of random typos. And I guess Atom removed some trailing whitespace.~~
- Moved into [REF] Fix miscellaneous typos in ReproIn heuristic file #396.

codecov · 2019-10-24T01:56:38Z

Codecov Report

Merging #387 into master will decrease coverage by 0.21%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
- Coverage   75.05%   74.84%   -0.22%     
==========================================
  Files          35       35              
  Lines        2806     2818      +12     
==========================================
+ Hits         2106     2109       +3     
- Misses        700      709       +9

Impacted Files	Coverage Δ
heudiconv/info.py	`100.00% <ø> (ø)`
heudiconv/heuristics/reproin.py	`81.11% <52.38%> (-1.58%)`	⬇️
heudiconv/convert.py	`78.81% <54.54%> (-0.67%)`	⬇️
heudiconv/external/dlad.py	`97.67% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09f7501...dce8e23. Read the comment docs.

tsalo · 2019-10-24T12:46:55Z

heudiconv/heuristics/reproin.py

+            if s.series_description.endswith('_SBRef'):
+                seqtype_label = 'sbref'


I am not sure how generalizable this is. Single-band reference scans are automatically outputted in an [X]_SBRef dicom folder with the CMRR MB sequence, where [X] is the name of the multi band scan.

note: for me, with whatever sequence we have on our Siemens now (yet to check), _SBRef is present only within SeriesDescription field, and not ProtocolName:

$> grep s4p2 000001.txt (0008,103e) LO [func_run-01_task-pinel_acq-s4p2-me_SBRef] # 40, 1 SeriesDescription (0018,1030) LO [func_run-01_task-pinel_acq-s4p2-me] # 34, 1 ProtocolName

and in reproin we "favor" (consider first) ProtocolName:

$> grep '^series_spec_fields' heudiconv/heuristics/reproin.py series_spec_fields = ('protocol_name', 'series_description')

I have ran into the same situation with other types of data, requiring to switch to use series_description in favor of protocol_name. I wonder how safe it would be to switch order generally. Otherwise reproin should just become smarter...

Should I open an issue about it in the ReproIn repository?

nah -- let's solve it HERE! Sorry for the delay. Absence of sbref handling in reproin started to bite me, so thank you for leading this effort here! ;)
I will push some tuneups shortly, but here is my question: When would we get _phase images, in particular for "bold"? Never saw those and do not see BIDS provisioning for them. BIDS talks about fmap "Case 2: Two phase images and two magnitude images" but that is something we do not get a grip in reproin -- to review multiple scans not independently from each other to decide on indexes (such as _phase1 and _phase2 here)

You get both _bold and _phase when you do Magnitude+Phase reconstruction. That's also when you get rec-magnitude and rec-phase sbrefs (as in #421). Phase is listed as one of the valid task MRI contrasts in the BIDS specification here.

THANK YOU! I was looking for _phase and thus didn't hit that BIDS spec link you gave.

yarikoptic · 2019-12-02T19:19:23Z

heudiconv/heuristics/reproin.py

                seqtype_label = 'pace'  # or should it be part of seq-
+            elif 'P' in s.image_type:
+                seqtype_label = 'phase'


this is already under seqtype == 'func' so if I read the code correctly, it wouldn't work for a simple fmap

I think a separate if statement needs to be added for seqtype == 'fmap' to check if 'P' in s.image_type. Does that sound right?

Wait, sorry, if seqtype is already known to be fmap then there's no reason to check s.image_type. I don't know if it's a problem though, since there's definitely a step that determines seqtype_label for field maps that's separate from this part. So this if statement should only apply to functional data.

heudiconv/heuristics/reproin.py

I think it would be a more correct behavior. Either it is func, or dwi, or likely anything else, we should get _sbref seqtype if it is present in series_description (not present in protocol name).

…_type_seqtype

Also issue a warning if none was figured out - I think this should not happen. It should make code a bit easier to read

yarikoptic · 2020-03-25T02:46:28Z

ok, I went a bit wild with my additional changes, but individual commits explain reasoning ;) I think sbref handling is special kind -- should just override the seqtype whatever it was decided on. So I moved it out from decision making. Please correct me if I am wrong. Meanwhile I will do a run on some dataset to verify that old behavior is preserved.

yarikoptic · 2020-03-25T13:18:31Z

eh, more to be done ... running on a sample with MB multi-echo data,

here is a difference from master

$> heudiconv/utils/test-compare-two-versions.sh heudiconv{-master,} --bids -f reproin --files dbic-misc/1075_sequence/sourcedata/sub-sid001567
Not running heudiconv since compare-versions/v0.7.0-1-g57c9c56 already exists
Running /home/yoh/picts/mris/heudiconv/venvs/dev3/bin/heudiconv with log in compare-versions/v0.7.0-12-ga5a8eba.log
Results differ: /home/yoh/picts/mris/compare-versions/v0.7.0-1-g57c9c56-v0.7.0-12-ga5a8eba.diff
 .heudiconv/sid001567/info/filegroup.json                                                      | 2144 +++++++++++++++++++++++++++++++-------------------------------
 .heudiconv/sid001567/info/heuristic.py                                                        |   65 +
 .heudiconv/sid001567/info/sid001567.auto.txt                                                  |    4 
 .heudiconv/sid001567/info/sid001567.edit.txt                                                  |    4 
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_bold__dup-01.dicom.tgz | 1985 ----------------------------------------------------------
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_sbref.dicom.tgz        | 2033 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_bold__dup-01.dicom.tgz    | 4275 -----------------------------------------------------------------------------------------------------------------------------
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_sbref.dicom.tgz           | 4260 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sub-sid001567/anat/sub-sid001567_acq-MPRAGE_T1w.json                                          |  682 -------------------
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_bold.json                         |  102 --
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_bold__dup-01.json                 |  162 ----
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_bold__dup-01.nii.gz               | 1891 -------------------------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_sbref.json                        |   68 +
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_sbref.nii.gz                      | 1891 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-1_bold__dup-01.json             |  133 ---
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-1_bold__dup-01.nii.gz           | 1389 ----------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-2_bold__dup-01.json             |  133 ---
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-2_bold__dup-01.nii.gz           | 1346 ---------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-3_bold__dup-01.json             |  133 ---
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-3_bold__dup-01.nii.gz           | 1331 --------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_rec-magnitude_echo-1_sbref.json      |  133 +++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_rec-magnitude_echo-1_sbref.nii.gz    | 1389 ++++++++++++++++++++++++++++++++++++++++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_rec-magnitude_echo-2_sbref.json      |  133 +++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_rec-magnitude_echo-2_sbref.nii.gz    | 1346 +++++++++++++++++++++++++++++++++++++++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_rec-magnitude_echo-3_sbref.json      |  133 +++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_rec-magnitude_echo-3_sbref.nii.gz    | 1331 ++++++++++++++++++++++++++++++++++++++
 sub-sid001567/sub-sid001567_scans.tsv                                                         |    8 
 task-pinel1_acq-s8XBoulder_bold.json                                                          |  101 --
 28 files changed, 13842 insertions(+), 14763 deletions(-)

I get those _rec-magnitude in multi-echo _sbref, which shouldn't be there...

tsalo · 2020-03-25T14:19:20Z

I get those _rec-magnitude in multi-echo _sbref, which shouldn't be there...

I think that is a pre-existing issue, given #422.

yarikoptic · 2020-03-25T16:05:18Z

Thank you @tsalo ! apparently I didn't have a cohesive picture of the situation! I guess we might just absorb and improve (according to my comment there) #422 into this PR to proceed. I will do it later today, and then redo testing

tsalo · 2020-03-25T16:16:58Z

I don't want to overcomplicate things, but #424 should also address the rec issue (see this comment).

…se images

… echoes

…ds_meta for consistency

* origin/master: (32 commits) ENH(TST): skip test_dlad.py if no datalad.api ENH: reproin validator config - ignore absence of custom column description [DATALAD RUNCMD] Update versions to the next version (as in changelog) helper makefile ENH: a helper to prepare release CHANGELOG for 0.7.0 BF: nipy#427 Sort of None echotime was not working. Replaced with nan DOC: basic doc about "-g custom" BF(PY<3.7): just sample the class of a compiled regex RF+ENH: allow matching study using regexp on study_description ENH: reproin - log applied substitutions ENH(TST): test application of "catch all" rule RF: reproin - remove binding of fixup data structures in function signatures RF+ENH: reproin - allow for an empty key in protocols2fix to provide fixups for any study fix: remove filtered DICOMs from file list enh: improve regression test fix: changes from code review RF: return back indexes in comments for seqinfo_fields Minor DOC (explicit list of values in numpy doc style) and remove ambiguity in reading comparison ci: remove 34 testing, add 37 ... Conflicts: heudiconv/convert.py - conflicted with using nan instead of None for EchoTime

yarikoptic · 2020-03-25T18:20:55Z

I don't want to overcomplicate things, but #424 should also address the rec issue (see this comment).

eh, I already overcomplicated this PR ;) Just pushed with additional tune ups, will do some testing now to see if we got stable grounds and then will review #424

yarikoptic · 2020-03-25T18:22:22Z

hm, for some reasons CI isn't running :-/

yarikoptic · 2020-03-25T19:46:15Z

FWIW -- current version within PR seems to do the right thing

$> heudiconv/utils/test-compare-two-versions.sh heudiconv{-master,} --bids -f reproin --files dbic-misc/1075_sequence/sourcedata/sub-sid001567
Not running heudiconv since compare-versions/v0.7.0-1-g57c9c56 already exists
Running /home/yoh/picts/mris/heudiconv/venvs/dev3/bin/heudiconv with log in compare-versions/v0.7.0-18-g22c571d.log
Results differ: /home/yoh/picts/mris/compare-versions/v0.7.0-1-g57c9c56-v0.7.0-18-g22c571d.diff
 .heudiconv/sid001567/info/filegroup.json                                                      | 2144 +++++++++++++++++++++++++++++++-------------------------------
 .heudiconv/sid001567/info/heuristic.py                                                        |   65 +
 .heudiconv/sid001567/info/sid001567.auto.txt                                                  |    4 
 .heudiconv/sid001567/info/sid001567.edit.txt                                                  |    4 
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_bold__dup-01.dicom.tgz | 1985 ----------------------------------------------------------
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_sbref.dicom.tgz        | 2033 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_bold__dup-01.dicom.tgz    | 4275 -----------------------------------------------------------------------------------------------------------------------------
 sourcedata/sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_sbref.dicom.tgz           | 4260 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_bold__dup-01.json                 |  162 ----
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_bold__dup-01.nii.gz               | 1891 -------------------------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_sbref.json                        |  162 ++++
 sub-sid001567/func/sub-sid001567_task-pinel1_acq-s8XBoulder_sbref.nii.gz                      | 1891 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-1_bold__dup-01.json             |  133 ---
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-1_bold__dup-01.nii.gz           | 1389 ----------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-1_sbref.json                    |  133 +++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-1_sbref.nii.gz                  | 1389 ++++++++++++++++++++++++++++++++++++++++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-2_bold__dup-01.json             |  133 ---
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-2_bold__dup-01.nii.gz           | 1346 ---------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-2_sbref.json                    |  133 +++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-2_sbref.nii.gz                  | 1346 +++++++++++++++++++++++++++++++++++++++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-3_bold__dup-01.json             |  133 ---
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-3_bold__dup-01.nii.gz           | 1331 --------------------------------------
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-3_sbref.json                    |  133 +++
 sub-sid001567/func/sub-sid001567_task-pinel2_acq-s4p2Xme_echo-3_sbref.nii.gz                  | 1331 ++++++++++++++++++++++++++++++++++++++
 sub-sid001567/sub-sid001567_scans.tsv                                                         |    8 
 25 files changed, 13933 insertions(+), 13881 deletions(-)

yarikoptic · 2020-03-25T20:07:17Z

FWIW I am of opinion that #424 is worthwhile on its own to follow this one, which is still relatively not that "revolutionary" ;) Meanwhile, I will push an amended last commit to hopefully trigger testing, and will do few more regression tests locally

…t be compatible with recent git annex

…ed bids portion has it

yarikoptic · 2020-03-26T02:26:55Z

ok, adjusted conditioning on the warning... still looking for a sample (small) dataset which we could use to add some basic testing.

edit 1: just shared a dataset with 200 dynamics and 3 echos on a phantom -- http://datasets.datalad.org/?dir=/dicoms/dartmouth-phantoms/cmrr-test . could come handy by seems to lack sbrefs :-/

yarikoptic · 2020-03-27T14:48:59Z

ok, not sure when/if I find a good dataset to do testing, so - let's merge and then proceed with #424 (I will take a stab at resolving conflicts later today, unless you @tsalo would stop me and take over ;))

tsalo added 3 commits October 23, 2019 07:33

Add SBRef and functional phase support to ReproIn.

616e479

Default func to BOLD.

95e3563

Merge remote-tracking branch 'nipy/master' into mbme-reproin

546c1e2

tsalo mentioned this pull request Oct 24, 2019

Handling of single-band reference volumes and functional phase data ReproNim/reproin#39

Closed

tsalo commented Oct 24, 2019

View reviewed changes

yarikoptic reviewed Dec 2, 2019

View reviewed changes

heudiconv/heuristics/reproin.py Show resolved Hide resolved

Revert typo fixes.

a38583f

tsalo mentioned this pull request Dec 3, 2019

[REF] Fix miscellaneous typos in ReproIn heuristic file #396

Merged

Merge remote-tracking branch 'nipy/master' into mbme-reproin

86f6d3a

yarikoptic mentioned this pull request Feb 20, 2020

ME func data: is having only magnitude _sbref a sign of a "problem"? #421

Closed

Merge remote-tracking branch 'nipy/master' into mbme-reproin

b221c32

tsalo requested a review from yarikoptic February 26, 2020 16:29

yarikoptic added 4 commits March 24, 2020 22:40

BF: reproin - make _SBRef considered regardless of seqtype

2264435

I think it would be a more correct behavior. Either it is func, or dwi, or likely anything else, we should get _sbref seqtype if it is present in series_description (not present in protocol name).

RF: reproin - adjusted the comment about P and M not decided at image…

be7ca60

…_type_seqtype

DOC: reproin - add TODO about phase1,2 etc

6b71733

RF+ENH: reproin - place all logic for "not seqtype_label" together

5b72404

Also issue a warning if none was figured out - I think this should not happen. It should make code a bit easier to read

yarikoptic assigned tsalo Mar 25, 2020

yarikoptic added this to the 0.7.1 milestone Mar 25, 2020

pvelasco and others added 4 commits March 25, 2020 12:41

Make sure we don't add _rec-magnitude for sbref when there are no pha…

0eb5fbd

…se images

ENH: issue a warning when number of suffixes isnt twice the number of…

2f3a7d2

… echoes

RF+ENH: avoid loading the same bids json twice, rename fileinfo -> bi…

6046dc0

…ds_meta for consistency

yarikoptic mentioned this pull request Mar 25, 2020

Don't add _rec-magnitude for sbref when there are no phase images #422

Closed

BF: boost minimal datalad version to 0.12.3 since older ones might no…

2c9e5ec

…t be compatible with recent git annex

yarikoptic force-pushed the mbme-reproin branch from 22c571d to 2c9e5ec Compare March 25, 2020 20:08

ENH: reproin - warning on empty suffix should not be issued if unpars…

dce8e23

…ed bids portion has it

yarikoptic merged commit 74d9140 into nipy:master Mar 27, 2020

tsalo mentioned this pull request May 9, 2020

Naming convention for "setter" scans for T1w/T2w ReproNim/reproin#40

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Support SBRef and phase data in ReproIn heuristic #387

ENH: Support SBRef and phase data in ReproIn heuristic #387

tsalo commented Oct 24, 2019 •

edited

Loading

codecov bot commented Oct 24, 2019 •

edited

Loading

tsalo Oct 24, 2019

yarikoptic Feb 20, 2020

tsalo Feb 26, 2020

yarikoptic Mar 25, 2020

tsalo Mar 25, 2020

yarikoptic Mar 25, 2020

yarikoptic Dec 2, 2019

tsalo Dec 2, 2019

tsalo Dec 3, 2019

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

tsalo commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

tsalo commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 26, 2020 •

edited

Loading

yarikoptic commented Mar 27, 2020

		if s.series_description.endswith('_SBRef'):
		seqtype_label = 'sbref'

ENH: Support SBRef and phase data in ReproIn heuristic #387

ENH: Support SBRef and phase data in ReproIn heuristic #387

Conversation

tsalo commented Oct 24, 2019 • edited Loading

codecov bot commented Oct 24, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

tsalo commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

tsalo commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 25, 2020

yarikoptic commented Mar 26, 2020 • edited Loading

yarikoptic commented Mar 27, 2020

tsalo commented Oct 24, 2019 •

edited

Loading

codecov bot commented Oct 24, 2019 •

edited

Loading

yarikoptic commented Mar 26, 2020 •

edited

Loading