Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] Rewrite the MRI/fieldmaps subsection for consistency with the rest of specs #651
[FIX] Rewrite the MRI/fieldmaps subsection for consistency with the rest of specs #651
Changes from all commits
648471c
ca5dcb8
6e05ad6
dd3c87f
d81d291
7533998
1c63137
9d1c49b
ea830e5
504dd30
353d346
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that confuses me about this template (even before this PR)- wouldn't we still have jsons for the magnitude maps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 canpybids
infer metadata from linked files?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't encountered any case where PyBIDS could not correctly query the metadata of fieldmaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had phase1/phase2/magnitude1/magnitude2 images, and I believe I had to duplicate the metadata across both phase and magnitude images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sbref
files are also optional, but not labeled so in the template.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm growing a feeling that
_sbref
should not exist altogether.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most people will not read that paragraph, especially if they feel they know the specification and are making a quick confirmatory look-up.
I agree with this. For this PR I see three alternatives:
[]
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).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.