[ENH] BEP001 - New entity: flip#672
Conversation
Pull upstream changes
- Modify `echo` entity respectively
effigies
left a comment
There was a problem hiding this comment.
LGTM. Cleaning yaml lint...
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
|
Thank you @effigies! Looks like all the CI requirements are satisfied now. |
src/schema/entities.yaml
Outdated
| If constituents of an entity-linked file collection differ as a | ||
| function of `EchoTime` acquisition parameter, the `_echo-<index>` key/value | ||
| pair MUST be used to distinguish individual files. |
There was a problem hiding this comment.
I think this revised definition is worded a little confusingly. Rather than "constituents of an entity-linked file collection", I would say "files that are part of an entity-linked file collection". Instead of "differ as a function of EchoTime", I think you could say "have different EchoTimes.
There was a problem hiding this comment.
Also, I think the definition should go before the usage and warning. Something like "The _echo-<index> key/value pair represents the EchoTime metadata field." Then you can go into when and how it should be used.
There was a problem hiding this comment.
I agree, re-wording this now.
|
I think rebasing on master should resolve the CI failures. |
|
@agahkarakuzu do you have the "allow edits from maintainers" box in the GitHub user interface ticked? I cannot seem to push to your fork's branch. (wanted to rebase on master) |
|
@sappelhoff I don't see that box on the PR page, probably because I am not an owner but a member. I'll try to solve that. |
|
@agahkarakuzu It's a checkbox when you create a new PR:
I don't know that it can be changed after the fact. |
|
@effigies it was not available to me while I was creating the PR neither. I can rebase to an upstream master clone branch on our fork, would that work? |
- Modify `echo` entity respectively
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
|
@effigies I gave it a try, hope that it won't mess it up. |
|
Last semantic change was Monday, so this can be merged this coming Monday if we get another review. Recommend squashing to keep things clean. |
|
Aaand it's in! Thanks a lot everyone and especially @agahkarakuzu for preparing the PR. |
|
Thank you @sappelhoff I will be sending PRs for the remaining entities this week, I think this was a good starting point towards merging bep001! |
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 splitBEP001pull requests into manageable parts.Dear BIDS community,
In #668, we proposed the common principle of
entity-linked file collections, which requires addition of several new entities (inv,flip,mt) for the common use cases of quantitative MRI applications. In this Part-3 of BEP001 pull requests, we would like to propose a new entity:flip*FlipAnglefile-collectionif the files differ as a function of theFlipAngleacquisition parameter.* We abstained from using its common abbreviation
fa, as it may be confused with theFA(fractional anisotropy) term fromdwirealm.We set the requirement level of this entity to
MUSTfor the applications falling within the definition ofentity-linked file collections, not for all the applications that may collect data at multipleFlipAnglevalues. This is because splitting files may not be the most convenient/preferred approach for some applications, such asASL#669. I took liberty to adjust the definition of theechoin that respect.I will be waiting for this PR to be discussed and resolved before sending PRs for the remaining entities
invandmt.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.