[ENH] Add part entity for complex-valued data

[tsalo]

Closes #367. This adds a new entity to distinguish magnitude and phase MRI data (except for BOLD and field map data, which have separate suffices for separating magnitude and phase).

The part entity is already proposed in BEP-001 and BEP-004, but applies to more than multi-contrast MRI and SWI (e.g., single-band reference images).

[sappelhoff]

Thanks for the proposal @tsalo. I am not too deep in the MRI related aspects of the spec, so let me summon @bids-standard/raw-mri for their opinions and reviews. :-)

[effigies]

I'm not opposed to this in principle, but in #429 it looks like it's being used to split files that are too large. We should probably settle that...

I have some exposure to complex-valued images, but not very comprehensive. Is it the case that you always get it in phase/magnitude (which don't have a single-file representation) as opposed to complex-float values (which do)? Or that you always work in the phase/magnitude space so having to convert to/from complex is bound to accumulate arithmetic error?

Just for reference, the types supported by NIfTI are:

#define DT_NONE                    0
#define DT_UNKNOWN                 0     /* what it says, dude           */
#define DT_BINARY                  1     /* binary (1 bit/voxel)         */
#define DT_UNSIGNED_CHAR           2     /* unsigned char (8 bits/voxel) */
#define DT_SIGNED_SHORT            4     /* signed short (16 bits/voxel) */
#define DT_SIGNED_INT              8     /* signed int (32 bits/voxel)   */
#define DT_FLOAT                  16     /* float (32 bits/voxel)        */
#define DT_COMPLEX                32     /* complex (64 bits/voxel)      */
#define DT_DOUBLE                 64     /* double (64 bits/voxel)       */
#define DT_RGB                   128     /* RGB triple (24 bits/voxel)   */
#define DT_ALL                   255     /* not very useful (?)          */


                            /*----- another set of names for the same ---*/
#define DT_UINT8                   2
#define DT_INT16                   4
#define DT_INT32                   8
#define DT_FLOAT32                16
#define DT_COMPLEX64              32
#define DT_FLOAT64                64
#define DT_RGB24                 128


                            /*------------------- new codes for NIFTI ---*/
#define DT_INT8                  256     /* signed char (8 bits)         */
#define DT_UINT16                512     /* unsigned short (16 bits)     */
#define DT_UINT32                768     /* unsigned int (32 bits)       */
#define DT_INT64                1024     /* long long (64 bits)          */
#define DT_UINT64               1280     /* unsigned long long (64 bits) */
#define DT_FLOAT128             1536     /* long double (128 bits)       */
#define DT_COMPLEX128           1792     /* double pair (128 bits)       */
#define DT_COMPLEX256           2048     /* long double pair (256 bits)  */
#define DT_RGBA32               2304     /* 4 byte RGBA (32 bits/voxel)  */

[tsalo]

In my experience, you get separate magnitude and phase files. Siemens sequences have a "Magn./Phase" reconstruction option that generally outputs separate DICOM folders for the two types.

Also, the tools I'm aware of for working with magnitude+phase data (e.g., phaseprep, t2star-mapping, and distortion correction in sdcflows) all seem to use separate files.

Obviously, you know more about how sdcflows works, so please correct me if tools like that can use complex-floats instead.

[effigies]

Yeah, I haven't seen anybody using complex values directly. If you haven't, it's probably best not to rock that particular boat.

[yarikoptic]

minor fix todo in light of #429 discussion

[tsalo]

I can't figure out why linkchecker is failing. @yarikoptic do you have any ideas?

[yarikoptic]

NB: copy/paste of the error would have expedited troubleshooting

URL        `https://www.w3schools.com/js/js_json_objects.asp'
Name       `JSON object'
Parent URL file:///root/build/site/02-common-principles.html, line 1136, col 18
Real URL   https://www.w3schools.com/js/js_json_objects.asp
Check time 0.062 seconds
Result     Error: SSLError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:727)

and indeed:

$> wget https://www.w3schools.com/js/js_json_objects.asp
--2020-05-05 14:00:23--  https://www.w3schools.com/js/js_json_objects.asp
Resolving www.w3schools.com (www.w3schools.com)... 192.229.173.207
Connecting to www.w3schools.com (www.w3schools.com)|192.229.173.207|:443... connected.
ERROR: The certificate of ‘www.w3schools.com’ is not trusted.
ERROR: The certificate of ‘www.w3schools.com’ has expired.
The certificate has expired

that is the cost of the "security" ;) url is reachable via insecure http://www.w3schools.com/js/js_json_objects.asp as well, I guess worth a PR fixing it up. Meanwhile twitted:

@wwwschools please address "expired certificate" for https://t.co/2jAeFoif1z (I found no better means of contacting, sorry)

— Yaroslav Halchenko (@yarikoptic) May 5, 2020

[robertoostenveld]

I do not understand (from a standards- and consistency perspective) why we need a new entity to distinguish magnitude and phase for certain MR contrasts, whereas we already have it in the specification for others. See https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#case-2-two-phase-images-and-two-magnitude-images

If we were to introduce part-<mag|phase>, is it subsequently also allowed to be used for BOLD contrast MR data? Would this ambiguity not lead to the risk of the specification becoming confusing?

I believe the proposal to use it here reveals inconsistency in the use of modality either specified in the directory name (as in sub-<label>/[ses-<label>/]<modality>/) versus it being specified at the end of the filename (as in_<modality>.nii.gz).

[yarikoptic]

Good observation @robertoostenveld ! I had similar reservation. And IMHO you are correct in:

I believe the proposal to use it here reveals inconsistency in the use of modality either specified in the directory name (as in sub-<label>/[ses-<label>/]<modality>/) versus it being specified at the end of the filename (as in_<modality>.nii.gz).

just with a note that <modality>/, there is possibly an additional level of confusion since in BIDS we use "modality" as higher level construct (func, anat, dwi) and which is a superset of possible data file "types", and then the only <modality thing is

$> git grep '<modality'                                                                             
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md:        sub-<label>[_ses-<label>][_acq-<label>][_ce-<label>][_rec-<label>][_run-<index>]_<modality_label>.nii[.gz]

As you rightfully pointed out there are already _magnitude{1,2} and _phase{1,2} for fmap modality . fmap could be an _dir-..._epi.nii.gz highlighting inconsistency. IMHO it is just an artifact of the BIDS evolution ;)

[tsalo]

If we were to introduce part-<mag|phase>, is it subsequently also allowed to be used for BOLD contrast MR data? Would this ambiguity not lead to the risk of the specification becoming confusing?

I have to apologize for this. I added OPTIONAL to the entity table for BOLD contrast MR data in 6bcc2a5, but that was a mistake. This was corrected in 2f67010, after your comment. Using part with BOLD contrast MR data should not be allowed.

I do not understand (from a standards- and consistency perspective) why we need a new entity to distinguish magnitude and phase for certain MR contrasts, whereas we already have it in the specification for others.

I agree that, from the perspective of a standard, this is not optimal. The existing suffices that distinguish magnitude and phase (i.e., bold/phase for func and phase{1,2}/phasediff/magnitude{1,2} for fmap) could probably have been consolidated if part and echo were used instead. However, I assume that field maps were incorporated into the specification before part was ever considered, and the choice to use _<bold|phase> instead of _part-<mag|phase>_bold was a conscious one made for practical reasons (see discussion in #128). We chose a separate suffix there because incorporating part for BOLD data would have caused problems for a lot of tools, which probably wasn't the best choice from the perspective of consistency.

Still, there are many other modalities that can have both magnitude and phase data, so I believe that there is a real need for a new entity to distinguish the two. If anything, I think that this would be an argument to remove existing suffices that should be superseded by the part entity, rather than an argument against adding the entity. I don't think that's going to happen, though, since at least the fmap suffices are so widely used. I also don't know what process, if any, exists to remove things from the specification.

EDIT: I just realized that, given the backwards-incompatibility of removing existing suffices, I could propose that they be removed in the BIDS 2.0 Google Doc if this PR is merged.

EDIT 2: I've proposed removing the relevant suffices in the BIDS 2.0 Doc.

[tsalo]

@yarikoptic Thank you for figuring out the linkchecker problem. It looks like it was fixed, since all tests are passing as of 2f67010.

[yarikoptic]

With the #429 being resolved by #460 via renaming _part into _split for annotation of splits of MEEG files, IMHO the horizon is clear now for this to proceed forward. Just some conflicts to be resolved first now (soon #475 will also make us to avoid such merge conflicts altogether ;) !)

[tsalo]

@yarikoptic It should be good now. I'm really excited to see this one move forward!

[robertoostenveld]

@tsalo thanks for clarifications. I agree.

If anything, I think that this would be an argument to remove existing suffices that should be superseded by the part entity, rather than an argument against adding the entity. I don't think that's going to happen, though, since at least the fmap suffices are so widely used. I also don't know what process, if any, exists to remove things from the specification.

I propose you make a note of this in the Suggestions for BIDS 2.0 document. Switching from 1.x to 2.0 would be the moment to make backward-incompatible changes.

[tsalo]

@robertoostenveld Agreed. I've added a new suggestion to the Google Doc. It's wayyy down at the very, very bottom.

[yarikoptic]

Well, may be a somewhat past-the-time idea/discussion point. I am ok also with merging as is, and then moving discussion into another issue/PR since it could easily be incremental.

[yarikoptic]

I am not sure if such detailed specification and expansion of different contrast_labels is needed here.
_bold is indeed a special case though since now it has a dedicated _phase. So now there cannot be another func contrast than _bold which would have its _phase.
NB ASL BEP005 seems decided to avoid func/ and went for a dedicted perf/ (perfusion) although its purpose AFAIK would be func/.
Unfortunately I did not follow the discussion in #128. I would have voted for any contrast_label to "assume" its default "composition" - either it is a mag or phase or combined if no _part is provided. Then #128 could have resulted in adding _part-phase and having explicit _part-phase_bold, opening possibilities for any other _part-phase_<contrast>.

Sure thing _part- makes no sense with _phase, but I think that is just a corner case which might be gone in 2.0.
So why not just to keep [_part-<mag|phase>]_<contrast_label> and just add a note to _phase in the table that _part then should not be specified. Also it might be worth starting adding "deprecation" statements on elements which will be removed in 2.0, but that would be a separate issue I guess ;)

[effigies]

LGTM. Now it's a race with #672.

[tsalo]

@yarikoptic's approval is fairly old now, so I'd like to get one more before initiating the five-day embargo period. Does that sound good to everyone?

Very old. Care for another look?

[sappelhoff]

LGTM overall, except a minor comment.

[tsalo]

🎉 It looks like we can start the embargo period. If there are no concerns before next Tuesday (11/24), I think we can merge this, right?

[sappelhoff]

It's been 6 days since the last interaction with this PR, and it has all approving reviews it needs, so I am merging now.

Thanks @tsalo

[oesteban]

Question: does this PR not affect fieldmaps at all? (I'm looking at https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#fieldmap-data)

Admittedly, the spec for fieldmaps is pretty unfortunate: we have eight suffixes (magnitude, magnitude1/2, phase1/2, phasediff, fieldmap, and epi) and most of them overlap with the new entity. Trying to apply the entity would end up producing pretty weird namings, because fieldmap and phase[1,2,diff] are not MR sequence names (and epi is actually the only one that will typically need only the magnitude).

This PR kind of pushes towards something like:

fmap/
   sub-1_part-mag_echo-1_gre.nii.gz       # gre = gradient-recalled echo; equiv. magnitude1
   sub-1_part-mag_echo-2_gre.nii.gz       # equiv. magnitude2
   sub-1_part-phase_echo-1_gre.nii.gz     # equiv. phase1
   sub-1_part-phase_echo-2_gre.nii.gz     # equiv. phase2
   sub-1_part-mag_sei.nii.gz              # sei = spiral-echo imaging; equiv. to magnitude
   sub-1_part-phase_sei.nii.gz            # equiv. to _fieldmap
   sub-1_part-mag_gre.nii.gz              # equiv. to _magnitude1 corresponding to phasediff
   sub-1_part-phase_gre.nii.gz            # equiv. to phasediff

Also, the units of these part-phase should be allowed to be given in rad, rad/s, or Hz (and could be an argument for ppm).

Another argument could be had to unify _sei and _gre as a unique suffix, if there were a metadata to indicate whether the image's range is clipped between -pi .. pi.

But this is backwards incompatible, so I guess (coming back to the beginning) that fieldmaps are unaffected by this PR?

[oesteban]

although, not breaking compatibility so much, we could imagine something like:

fmap/
   sub-1_part-mag_echo-1_phasediff.nii.gz       # equiv. magnitude1
   sub-1_part-mag_echo-2_phasediff.nii.gz       # equiv. magnitude2
   sub-1_part-phase_echo-1_phasediff.nii.gz     # equiv. phase1
   sub-1_part-phase_echo-2_phasediff.nii.gz     # equiv. phase2
   sub-1_part-mag_phasediff.nii.gz              # equiv. to _magnitude1 corresponding to phasediff
   sub-1_part-phase_phasediff.nii.gz            # equiv. to phasediff
   sub-1_part-mag_fieldmap.nii.gz               # equiv. to magnitude
   sub-1_part-phase_fieldmap.nii.gz             # equiv. to _fieldmap

[tsalo]

I have an open issue about that for BIDS 2.0: bids-standard/bids-2-devel#3

I had assumed that the new suffix would just be epi, but phasediff sounds good too.

EDIT: The general idea was to remove the extraneous suffixes in 2.0, but keep everything as-is in the current major version for backwards compatibility.

[oesteban]

Feels a bit awkward to modify all other MRI instances that almost always use just the magnitude (80/20) and keep the subset where the 80/20 is reversed (i.e., 80% would be better off with part-).

[agahkarakuzu]

@oesteban although we were lucky to start from a clean slate and had no concerns about retro-compatibility, I'd like to leave my two cents about how we approached this for RF fieldmaps (B1+ and B1-):

To get rid of the ambiguity caused by the use of suffixes attaining common sequence names (e.g. gre) or terms (e.g. phase), we introduced the common concept of entity-linked file collections. These collections make use of suffixes that can clearly describe the intended application of a set of files (e.g. DEB0 instead of generic gre for double-echo GRE based B0 mapping) when coupled with linking entities (echo, part, flip etc.) relevant to the process.

Similar to that in B0 mapping, there are various B1 mapping methods revolving around alternating certain acquisition parameters. The file-collection approach enabled describing several B1 mapping methods.

When descriptive suffixes are available in the context of file collections, the use of part (and other linking entities) ease the definition of quantitative MRI inputs with clarity. I know retro-compatibility is a stumbling block for such changes at the moment, but if it is no longer an issue from v2.0 on, I'd say part entity would be a great help for organizing B0 map inputs with file collections in mind.

[effigies]

That fieldmaps are the most common case where part- would be most clarifying actually makes the transition harder. If we do that, then tools are going to need to support old- and new-style fieldmap names. So sdcflows, which aims to support the four types of fieldmaps would need to support seven naming conventions.

With _phase.nii, it was a pretty niche case and recently added, so deprecating it for part-phase_bold.nii affects quite few datasets and (probably) fewer tools.

Still, I think it might be a good idea to put this to the community. Deprecating 3 out of 4 existing fieldmap conventions would cause overhead for tools, but it would prod dataset curators toward a more coherent convention for their fieldmaps and ease the transition to BIDS 2.

[oesteban]

@tsalo @agahkarakuzu @effigies thanks very much for your explanations, it makes sense - just wanted to be explicit as regards that leaving the fieldmaps part of the spec untouched. As Chris mentioned, I see the compromise we need to achieve between keeping tools updated within BIDS 1 but making coherent steps for a smooth transition towards BIDS 2.

Question for @agahkarakuzu, the B1 mapping methods you referenced, are those artifacts stored under fmap/?