-
Notifications
You must be signed in to change notification settings - Fork 169
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
[ENH] BEP001 - qMRI maps and some additional metadata #690
[ENH] BEP001 - qMRI maps and some additional metadata #690
Conversation
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
Yes, please include those suffixes in that file. It doesn't have to happen in this PR if you consider it out-of-scope, but it should happen at some point. |
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
Thank you @tsalo, which command should I run to generate the entity table? I will soon address all the comments you made above. |
Here is what you'll need to run: bids-specification/tools/bids_schema.py Lines 453 to 454 in ad73668
However, if by some miracle we get #610 merged before this PR, you won't need to run anything at all. |
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
I'd love to get that merged, but it may be more reasonable to do once the ASL PR is merged. (and then a proper rebasing and getting the #610 pr to date again is done) WDYT? The other option would be to merge #610, but then to make lots of new formatting requests to the ASL PR. Not sure here what would be best. |
@sappelhoff Perhaps we can try to fit it between ASL and PET, then? Not sure how that will interact with qMRI, but it's not a big deal if it comes after. |
Yes, we should definitely do it BEFORE PET 👍 |
@tsalo @sappelhoff I run the |
@agahkarakuzu I'm going to push some small styling fixes in the next few minutes that should fix at least some tests, if you want to wait before committing more... |
Sure @effigies, please go ahead. I will look at those changes and try to watch out for them in upcoming commits. |
The build failures are due to a reference to |
@tsalo @effigies @sappelhoff I can add |
I will defer to their judgment. I haven't read much yet, so don't have a sense of the scope. I was just trying to get it so it would build and I could read the rendered version before commenting on the source. |
I think that'd be manageable, as it's all contained in a single file with all additions (no complex deletions/additions/replacements) |
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.
Okay, I've looked through it and though it's large, it's quite focused. I think we could include an appendix without overburdening the PR.
Some comments at this point.
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
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.
LGTM.
I just have a couple of questions and flagged typos remaining, although they're all collapsed together in the Conversations tab. |
@tsalo I collapsed all the comments (especially the ones with minor fixes) with commit hashes. I’ll go through them again to see which questions/suggestions I missed. |
Went back to look at the remaining issues, so while I was at it, I summarized (and expand slightly) @tsalo's comments so they an be at the front of the conversation:
|
Thank you @effigies, this was really helpful! I went through the list and addressed them. Items 2, 3 and 4 are quite similar, collectively suggesting to represent all the suffixes in the |
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.
LGTM
@agahkarakuzu @tsalo @Gilles86 I'm inclined to squash this into a single commit, but if any of you would prefer the whole history be preserved in the git tree, I can do a simple merge. |
afaic you can just squash it :) |
@effigies I+1 for squash-merge |
so, does that conclude BEP001? :-) how can we celebrate this properly? |
@sappelhoff hard to believe, but yes! I made myself some espresso to read rendered version on |
Multiple measurements of whiskey? |
Headnote
A while ago we started #508, where you can find information on the overall purpose of
BEP001
. The PR #508 was closed as discussions called for some major revisions and to splitBEP001
pull requests into manageable parts.Dear BIDS community,
So far 4 BEP-001 PRs have been merged in the scope of
1.5.0
milestone. These PRs introduced i) a new common principlefile-collections
, ii) the entities it will use (inv
,mt
,flip
;part
andecho
were present) and iii) (finally) resolved the long-discussedRepetitionTime
issue.Drawing upon these recent additions, in this Part-5/6 of BEP001 pull requests, we would like to propose including a list of (19) parametric maps and to clean up the existing table of anatomical images, which lacked descriptions and contained some ambiguous suffixes. Lastly, we added some metadata fields.
Thanks to the concept of the
file collections
and theqMRI appendix
(to be added), I think this PR is going to bring a lot of new use cases and clarity to the anatomy imaging data without inflating the section itself too much.Relevant (minor) changes
NumberShots
and fix some typosfile collections appendix
to clarify naming of files in a file collection.modality_label
withsuffix
in the anat template as the termmodality
becomes overloaded otherwise.anat.yml
Before updating the
/schema/datatypes/anat.yml
further, I wanted to have @tsalo 's opinion about whether or not to include file-collection associated suffixes (e.g.MPM
,MP2RAGE
) and entities (flip
,inv
,mt
) to this file. I did not run thebids_schema
yet to update the entity table.On behalf of the BEP001 core team:
Gilles de Hollander (@Gilles86), Alberto Lazari (@lazaral), Christophe Phillips (@ChristophePhillips), Kirstie Whitaker (@KirstieJane), Tibor Auer (@tiborauer).
Best regards.