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] allow _dir for other EPI (func, dwi) sequences #78

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

yarikoptic
Copy link
Collaborator

Closes #47

This commit also demonstrates ability to reference other sections within the
document (when referencing fmap Case 4 where _dir described in greater detail
ATM

@chrisgorgo
Copy link
Contributor

Pinging @bids-standard/bep_leads for comments.

@satra
Copy link
Collaborator

satra commented Oct 30, 2018

func already has a few ways of disambiguating (run, task, etc.,.). as such my preference would be to start nudging people to get metadata from the sidecar instead of encoding in filename. perhaps we could go the other way and remove dir from dwi and leave it in the json sidecar.

it almost seems like we just want a disambiguation mnemonic. but i feel that naming things in filenames should be as restrictive as possible.

alternatively, we can adopt more generally what we discussed in derivatives meeting that any key and value can be added to the filename in pairs, but in general only a disambiguation mnemonic suffices.

@yarikoptic
Copy link
Collaborator Author

While doing it I thought that may be detailed descriptions for all those key/pairs shared by different modalities could be brought into a single location, where it would be easy to reference them. Eg as subsections of src/99-appendices/04-entity-table.md ?

@yarikoptic
Copy link
Collaborator Author

hm, ... wise words @satra ;-)
Indeed I do not generally envision people to perform identical task twice (thus kinda demanding the same task and run) just for the sake of having both field encoding directions. I am (re)thinking...

@chrisgorgo
Copy link
Contributor

@yarikoptic what's the status after @satra comments? Would you be ok closing this PR?

@yarikoptic
Copy link
Collaborator Author

I thought / looked at it a bit more and was provided some counter examples, e.g. HCP dataset where the same task would have LR and RL "runs". Sure thing it could be encoded within _run but again that would loose the specificity/semantic which _dir carries.
Other "examples" include some studies where they could have changed direction within the study/across sessions (ppl do crazy stuff out there in the wild ;-)). Without reflecting it in the name, it would become not obvious (not caught by the bids-validator as well?) that such variability is present.
So I still feel that _dir, if desired, should be allowed for since it provides a clear semantic on how otherwise possibly identical (within or across subjects/sessions) files differ. Stuffing that information into run (if applicable at all) or acq would retain the same filename length while loosing clear semantic association.

Please let me know what you think @chrisfilo and @satra

@chrisgorgo
Copy link
Contributor

I still don't think it's necessary and acq is the right place to put it (not run since it's restricted to numbers), but I don't feel strongly enough about the topic to block this change. It also been three weeks and with no further feedback from the community - this does not seem like a breaking change.

@ericearl
Copy link
Collaborator

ericearl commented Nov 20, 2018

@yarikoptic I have seen that too and while not ideal, it is what happens in the wild. Though the sidecar JSON should have things like PhaseEncodingAxis and PhaseEncodingDirection, the filenames up top would appear as two of the same runs though they could be more easily differentiated by just a more appropriate _dir- label.

@yarikoptic
Copy link
Collaborator Author

Thank you @ericearl . Taking it with @kaczmarj 's comment at #47 (comment) I think there is enough community support/interest to see it implemented. I will refurbish this PR for merge

See bids-standard#47 for initial
brief discussion.

This commit also demonstrates ability to reference other sections within the
document (when referencing fmap Case 4 where _dir described in greater detail
ATM
@yarikoptic
Copy link
Collaborator Author

ready...
now might cause conflicts with #93 I chased it with

@chrisgorgo
Copy link
Contributor

Thanks!

@chrisgorgo chrisgorgo merged commit 7019da5 into bids-standard:master Nov 21, 2018
@yarikoptic yarikoptic deleted the enh-dir branch December 5, 2018 17:23
@sappelhoff sappelhoff changed the title ENH: allow _dir for other EPI (func, dwi) sequences [ENH] allow _dir for other EPI (func, dwi) sequences Sep 8, 2020
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.

BEP: make dir optional for other than fmap modalities
4 participants