[FIX] Rewrite the MRI/fieldmaps subsection for consistency with the rest of specs

[oesteban]

The fieldmaps specification was really particular, not abiding by the formatting and style of other MRI sections.

This PR is meant as a refactor, just clarifying some aspects and standardizing the section before #622 is finally addressed.

[tsalo]

Including brackets around the whole filename to indicate that it's an optional file isn't used consistently across the specification. I think it really only shows up in the MEG/EEG sections, so I'm not sure if we want to introduce it here.

Also, the schema isn't capable of storing that kind of information at the moment, so those brackets will be removed when/if #610 is merged.

[oesteban]

Not sure what the alternative would be. Any suggestions?

[sappelhoff]

Not sure what the alternative would be

I think it'd be fine not to communicate requirement level in the "template" section. After all, we are also not distinguishing between recommended and required files either. Users can see which files are optional from the spec text.

[oesteban]

I'm not particularly attached to this indication of an optional data object, but I think there are two different discussions here.

1. Whether the schema can store this information at the moment -- here the question is whether the schema should store this information, actually. I guess so, because the validator needs to know whether magnitude2 must be found (phase1/2 fieldmaps) or is optional (phasediff fieldmaps).
2. Whether the template is the right place to state the indication

IMHO, 1) is harder to address, and outside the scope of this PR -- I guess that merging #610 will have to address this question, and if that means that the brackets in the template are gone because some other mechanism implements this, I'm fine by that (that's how BIDS evolves, anyways).

Then 2) - I think that the Template is basically what most of the people check when preparing their data. Therefore, I think this is the most effective way of expressing that this element is optional. It also needs to be clearly stated within the text, but considering that magnitude2 may and may not be optional (phasediff and phase1/2, respectively), there is a larger chance this is lost in some future refactor. For clarity, I think having the template annotated with requirement levels, especially when irregular, is very useful.

@tsalo: would something like:

        sub-<label>[_ses-<label>][_acq-<label>][_run-<index>]_magnitude2.nii[.gz]  # OPTIONAL

work out?

[tsalo]

There really isn't any mechanism for stating that some suffixes or extensions are optional in combination with others in the schema at the moment, so #610 will erase any indications of that from templates and users will need to rely on the text for that kind of information. I don't think it's that big a deal to address this issue in #610 though.

[oesteban]

What would you prefer then for this PR?

        [sub-<label>[_ses-<label>][_acq-<label>][_run-<index>]_magnitude2.nii[.gz]]

or

        sub-<label>[_ses-<label>][_acq-<label>][_run-<index>]_magnitude2.nii[.gz]  # OPTIONAL

I'm leaning towards the # OPTIONAL annotation which is more visible and obvious - WDYT?

[tsalo]

I think # OPTIONAL works well.

[oesteban]

This is ready for review

cc/ @bids-standard/raw-mri-anat @bids-standard/raw-mri-dwi @bids-standard/raw-mri-func

[oesteban]

Particularly interested in @chrisgorgo's opinion on my commit (dd3c87f), as I remember some debate when dir-<label> was added to the standard.

[tsalo]

I have some minor changes, but this is looking really good. Thank you for doing it!

[tsalo]

One thing that confuses me about this template (even before this PR)- wouldn't we still have jsons for the magnitude maps?

[oesteban]

The magnitude maps are part of the acquisition (as the phase maps are), so all the metadata is probably best stored only on the phase information files. I'm not sure what metadata could possibly exist that only applies to the magnitude (and doesn't to the phase).

[tsalo]

That's true, but I don't think that kind of relationship is supported in the specification. I guess we would call it something like horizontal inheritance?

I could be wrong though.

[tsalo]

For tools like pybids, I think you need sidecar files for the magnitude images in order to extract their metadata. @effigies is that still the case or can pybids infer metadata from linked files?

[oesteban]

I haven't encountered any case where PyBIDS could not correctly query the metadata of fieldmaps.

[effigies]

I had phase1/phase2/magnitude1/magnitude2 images, and I believe I had to duplicate the metadata across both phase and magnitude images.

[oesteban]

Care for a couple of green ticks?

cc/ @bids-standard/raw-mri-anat @bids-standard/raw-mri-dwi @bids-standard/raw-mri-func

[effigies]

sbref files are also optional, but not labeled so in the template.

Suggested change

	sub-<label>[_ses-<label>][_acq-<label>][_run-<index>]_magnitude2.nii[.gz] # OPTIONAL
	sub-<label>[_ses-<label>][_acq-<label>][_run-<index>]_magnitude2.nii[.gz]

[oesteban]

I think it is important to mark optional elements on the template. Following that argument all fieldmaps are essentially optional because a BIDS-valid dataset may or may not have them.

There's been some discussion about this annotation with @tsalo. Could you read through that and then let us know if you still think this change should be made (i.e., remove the OPTIONAL annotation in the template)?

[oesteban]

Also, I'm growing a feeling that _sbref should not exist altogether.

[effigies]

Agreed that all files are optional, so it seems strange to mark one out as super-extra optional in either form. (I did find and mark unresolved that comment thread, btw.)

Immediately below the template is:

where the REQUIRED _phasediff image corresponds to the phase-drift map between echo times, the REQUIRED _magnitude1 image corresponds to the shorter echo time, and the OPTIONAL _magnitude2 image to the longer echo time.

I don't understand why we need to add novel markup to the template when the text is right there.

Just as a final point, I would say that I'm not opposed to the concept of indicating in the template that one file in a group is optional, but I am opposed to introducing it in a one-off manner. I would rather we made a principled decision that takes into account all such groupings and applies it to the entire spec in one PR.

That said, I won't block this PR over this quibble.

[oesteban]

I don't understand why we need to add novel markup to the template when the text is right there.

Most people will not read that paragraph, especially if they feel they know the specification and are making a quick confirmatory look-up.

Just as a final point, I would say that I'm not opposed to the concept of indicating in the template that one file in a group is optional, but I am opposed to introducing it in a one-off manner. I would rather we made a principled decision that takes into account all such groupings and applies it to the entire spec in one PR.

I agree with this. For this PR I see three alternatives:

1. Leave it as is now and come back to the issue later (or within [SCHEMA] Render schema elements in text #610)
2. Roll it back to [] since that nomenclature, even though not formalized, is apparently present elsewhere in BIDS (and finally address it within [SCHEMA] Render schema elements in text #610).
3. Remove any annotations (and bring them up in a separate PR or [SCHEMA] Render schema elements in text #610).

I think 3 would be the best option, but I'm afraid that this will concept will be probably lost (I'm honestly not willing to go through the whole spec to propose something in a separate PR or in #610). So, after the discussion with @tsalo I think I prefer option 1.

[effigies]

I had phase1/phase2/magnitude1/magnitude2 images, and I believe I had to duplicate the metadata across both phase and magnitude images.

[oesteban]

Can I ask for one more blessing?

[effigies]

5 business days since last change and 2 positive reviews. Merging.

Thanks for this effort, @oesteban!