Skip to content
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

[SCHEMA] Update qMRI fieldmap schema #728

Merged
merged 8 commits into from
Feb 12, 2021
Merged

[SCHEMA] Update qMRI fieldmap schema #728

merged 8 commits into from
Feb 12, 2021

Conversation

effigies
Copy link
Collaborator

This PR:

  1. Adds a number of fieldmaps that are in the spec but not the schema:
    • TB1AFI
    • TB1TFL
    • TB1RMF
    • RB1COR
    • TB1SRGE
  2. Allows inv- entities for all qMRI fieldmaps, in accordance with https://bids-specification.readthedocs.io/en/latest/99-appendices/10-file-collections.html#fieldmap-data
  3. Renames group comments more informatively.

I notice https://bids-specification.readthedocs.io/en/latest/99-appendices/10-file-collections.html#fieldmap-data also permits mt-, but I don't see any examples or use cases that call for it. Is it supposed to be permitted?

Clarifying these issues here, and will update the validator according to any decisions made.

cc @ChristophePhillips @Gilles86 @agahkarakuzu @tsalo

@tsalo tsalo added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Feb 10, 2021
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like TB1AFI requires acq for the two TRs. TB1TFL and TB1RMF seem to require acq for something more complicated. RB1COR requires acq for coils, although I think that could be replaced with the coil/ch entity when we get that added.

I'm not sure if the acq entity is required for all of them, since the text is a little ambiguous, but it does seem required. Would be good to get the qMRI folks' input on that.

@effigies
Copy link
Collaborator Author

Yes, the text says "should", not required, so I left it as optional for now.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that the specification text is accurate w.r.t. acq's requirement level, these changes look good to me.

Normalizing to the most common. Cannot find a canonical reference outside of BIDS, so it may be backward.
@effigies
Copy link
Collaborator Author

Added ab25456 so that all my questions/suggestions for the BEP-001 contributors about intent are in a single place.

@agahkarakuzu
Copy link
Contributor

Thank you @effigies I'll go through them tomorrow.

@agahkarakuzu
Copy link
Contributor

I'm not sure if the acq entity is required for all of them, since the text is a little ambiguous, but it does seem required. Would be good to get the qMRI folks' input on that.

@tsalo the acq was the only option to describe some of the fieldmaps. They are required in practice, but I avoided it given that acq is freeform. So I used the requirement level should. There is explanation about this in the appendix text, so if optional is a better selection for the schema, that's all good. As you pointed out, we can update these when there are new entities.

From the files changed, I see there was a confusion about between TB1RMF an TB1RFM. I also noticed that there was a typo about this int he appendix as well. The correct form is TB1RFM, so I can unify them to this similar to what @effigies did in 9672f91.

The other files changed looks good overall, now I'll review them one by one. Thank you!

src/99-appendices/11-qmri.md Outdated Show resolved Hide resolved
src/schema/datatypes/fmap.yaml Outdated Show resolved Hide resolved
src/schema/datatypes/fmap.yaml Show resolved Hide resolved
@effigies
Copy link
Collaborator Author

There is explanation about this in the appendix text, so if optional is a better selection for the schema, that's all good.

Right now the schema only distinguishes required and optional, and doesn't have a notion of recommended. So we'll keep it as optional.

The correct form is TB1RFM

Fixed in 3bfeabb.

@agahkarakuzu
Copy link
Contributor

agahkarakuzu commented Feb 12, 2021

Thanks @effigies! Can I push changes to this branch for other (relevant) corrections in the appendix?

@effigies
Copy link
Collaborator Author

Can I push changes to this branch for other (relevant) corrections in the appendix?

Sure.

@agahkarakuzu
Copy link
Contributor

@effigies I noticed that I don't have write access. If that's how it should be, then I can open a PR from a fork.

@effigies
Copy link
Collaborator Author

It's just mass change fa- to flip-? I can do that real quick.

@agahkarakuzu
Copy link
Contributor

image

image

image

Yes, these are the changes

src/99-appendices/11-qmri.md Outdated Show resolved Hide resolved
@effigies
Copy link
Collaborator Author

Tests passing. Are we good to merge?

@effigies effigies merged commit c8f5827 into master Feb 12, 2021
@effigies effigies deleted the schema/qmri_fmaps branch February 12, 2021 20:31
@sappelhoff sappelhoff changed the title Update qMRI fieldmap schema [SCHEMA] Update qMRI fieldmap schema Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants