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

[ENH] BEP001 - New entity: flip #672

Merged
merged 17 commits into from
Nov 23, 2020

Conversation

agahkarakuzu
Copy link
Contributor


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 split BEP001 pull 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:

Entity Corresponding metadata Purpose
flip* FlipAngle To distinguish constituents of a file-collection if the files differ as a function of the FlipAngle acquisition parameter.

* We abstained from using its common abbreviation fa, as it may be confused with the FA (fractional anisotropy) term from dwi realm.

We set the requirement level of this entity to MUST for the applications falling within the definition of entity-linked file collections, not for all the applications that may collect data at multiple FlipAngle values. This is because splitting files may not be the most convenient/preferred approach for some applications, such as ASL #669. I took liberty to adjust the definition of the echo in that respect.

I will be waiting for this PR to be discussed and resolved before sending PRs for the remaining entities inv and mt.

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.

- Modify `echo` entity respectively
tsalo
tsalo previously requested changes Nov 16, 2020
src/99-appendices/09-entities.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM. Cleaning yaml lint...

src/schema/entities.yaml Outdated Show resolved Hide resolved
src/schema/entities.yaml Outdated Show resolved Hide resolved
src/schema/entities.yaml Outdated Show resolved Hide resolved
agahkarakuzu and others added 2 commits November 16, 2020 15:42
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@agahkarakuzu
Copy link
Contributor Author

Thank you @effigies! Looks like all the CI requirements are satisfied now.

Comment on lines 105 to 107
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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, re-wording this now.

@effigies
Copy link
Collaborator

I think rebasing on master should resolve the CI failures.

@sappelhoff
Copy link
Member

@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)

@agahkarakuzu
Copy link
Contributor Author

@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.

@effigies
Copy link
Collaborator

@agahkarakuzu It's a checkbox when you create a new PR:

Screen Shot 2020-11-19 at 09 56 41

I don't know that it can be changed after the fact.

@agahkarakuzu
Copy link
Contributor Author

agahkarakuzu commented Nov 19, 2020

@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?

@agahkarakuzu
Copy link
Contributor Author

@effigies I gave it a try, hope that it won't mess it up.

@effigies effigies added this to the 1.5.0 milestone Nov 20, 2020
@effigies
Copy link
Collaborator

Last semantic change was Monday, so this can be merged this coming Monday if we get another review.

Recommend squashing to keep things clean.

@sappelhoff sappelhoff merged commit ddc16b7 into bids-standard:master Nov 23, 2020
@sappelhoff
Copy link
Member

Aaand it's in! Thanks a lot everyone and especially @agahkarakuzu for preparing the PR.

@agahkarakuzu
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants