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

Add functional phase data patterns to file_level_rules #835

Merged
merged 3 commits into from
Sep 17, 2019

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Sep 17, 2019

Closes #834. Adds support for functional phase data (added to the BIDS specification in bids-standard/bids-specification#128 and bids-standard/bids-specification#312).

@codecov
Copy link

codecov bot commented Sep 17, 2019

Codecov Report

Merging #835 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #835   +/-   ##
=======================================
  Coverage   89.08%   89.08%           
=======================================
  Files          81       81           
  Lines        2328     2328           
  Branches      427      427           
=======================================
  Hits         2074     2074           
  Misses        218      218           
  Partials       36       36

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0745c4d...fc0f20a. Read the comment docs.

@effigies
Copy link
Collaborator

I would clump the suffixes together. bold, sbref, cbv and phase are all clustered on the big table, so let's put them together here.

Beyond this, @rwblair @nellh are there further considerations for adding a suffix?

@tsalo
Copy link
Member Author

tsalo commented Sep 17, 2019

@effigies Done!

In reordering the tokens I noticed that _defacemask is included as a func token even though it's only included under anat in the Entity table. I can open a related issue either here (if it should be under anat) or in the specification repository (if the Entity table is wrong), but I wasn't sure which way it should go.

@rwblair
Copy link
Member

rwblair commented Sep 17, 2019

I can't seem to find a reference to defacemasks for functional images in the standard even outside the entity table. If its removed then we can simplify the whole section by adding a 'func_ext' like other sections have for matching .nii nii.gz and .json, defacemask is the only one missing .json extension.

@effigies
Copy link
Collaborator

Perhaps check the git blame to see if there was a use case driving its inclusion?

@rwblair
Copy link
Member

rwblair commented Sep 17, 2019

Looks like its origins are in #233. Implies that at some point in the spec that func defacemasks were acceptable.

@rwblair
Copy link
Member

rwblair commented Sep 17, 2019

ds000113 has funcitonal defacemasks for every run.
ds001107 is using them as well

So I guess well keep them in the validator for now. I'll keep looking to see if there is any further discussion around this.

@tsalo
Copy link
Member Author

tsalo commented Sep 17, 2019

Okay, that sounds good.

Regarding supporting functional phase data, is there anything else that needs to be changed? It seems to be working locally on my own complex (phase+mag), multi-echo data.

@rwblair
Copy link
Member

rwblair commented Sep 17, 2019

@tsalo one last thing, could you add an example of a valid filename to bids-validator/tests/type.spec.js around line 41 to have it covered by some sort of test?

@rwblair rwblair merged commit 392af3c into bids-standard:master Sep 17, 2019
@tsalo tsalo deleted the add-phase branch September 17, 2019 18:23
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.

Support phase functional MRI data
3 participants