Skip to content

[ENH] - Remove single fmap-type assumption for BIDSIFY, more flexible derivative specification#291

Merged
jerdra merged 6 commits intoTIGRLab:masterfrom
jerdra:bidsify_keep
Nov 16, 2020
Merged

[ENH] - Remove single fmap-type assumption for BIDSIFY, more flexible derivative specification#291
jerdra merged 6 commits intoTIGRLab:masterfrom
jerdra:bidsify_keep

Conversation

@jerdra
Copy link
Copy Markdown
Contributor

@jerdra jerdra commented Nov 10, 2020

Changes:

  • Can now have multiple fmap types determined by the (modality, acq) tuple. These will be independently matched to scans
  • "inherit" spec under "alt" implemented allowing source files to augment the bids fields of their derived children
  • Fixed case where if the derivative of a file is under a different class, then the destination directory needs to be updated
  • When derivatives were specified they were selected in lieu of the parent scan. The update now allows you to specify derivatives in addition to the parent by simply specifying the derivative to have a type that is the parent's type. This means you can have the parent in addition to its derivative pushed into the BIDS directory instead of just the latter. (Useful for boldref/sbref cases)
  • is_ref option added which denotes that the scan is a reference scan and that BIDS info about this particular scan needs to be supplemented with info from the next scan (task)

Examples:

SBREF:{...

bids:{
    is_ref: true,
    alt:[
         {
             template: "<TEMPLATE>",
             type: FMRI-DPA,
             inherit: {acq: sbref},
         },
         {
              type: SBREF
         }
        ]
}

This specifies:

  • SBREF as a reference scan (task info is supplemented by the next scan)
  • SBREF has a derivative of type FMRI-DPA, in addition the child will inherit the acq-sbref property
  • SBREF will also have itself as a child so that in addition to the child being pushed, it itself will also be pushed

FIX

  • Grouped scans no longer assigned to separate fmaps due to the series number alone being used for mapping to fmaps.

This means, for example, a series of resting state scans will always be matched to the same fmap and not to different ones if it happens-so that the last resting state scan series number was closer to another fmap

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Nov 10, 2020

Hello @jerdra, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 datman.

Comment last updated at 2020-11-16 18:31:56 UTC

@jerdra
Copy link
Copy Markdown
Contributor Author

jerdra commented Nov 10, 2020

will fix PEP issues tmrw!

@jerdra
Copy link
Copy Markdown
Contributor Author

jerdra commented Nov 11, 2020

@DESm1th @josephmje @benselby ready to review!

@jerdra jerdra marked this pull request as draft November 11, 2020 18:36
@jerdra
Copy link
Copy Markdown
Contributor Author

jerdra commented Nov 11, 2020

I just dreamt up a bug w/how the series numbers are used to match fmaps. Gonna push a small adjustment to that algo and we should be good...

i really need to write tests for bidsify one day..

@DESm1th
Copy link
Copy Markdown
Contributor

DESm1th commented Nov 11, 2020

Haha, thanks for being thorough Jer! <3

@jerdra
Copy link
Copy Markdown
Contributor Author

jerdra commented Nov 16, 2020

thanks for the suggestion @DESm1th, i've updated the matching algo and ran some test data through with success. It's ready to review!

@josephmje @benselby

@jerdra jerdra marked this pull request as ready for review November 16, 2020 17:15
DESm1th
DESm1th previously approved these changes Nov 16, 2020
Copy link
Copy Markdown
Contributor

@DESm1th DESm1th left a comment

Choose a reason for hiding this comment

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

Looks good Jer! Thanks for updating this :)

Comment thread bin/bidsify.py Outdated
alts = []
for d in alt:
'''
Allow "self" to propogate itself if the tag itself
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this comment got a bit mangled haha

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hahaha wellppp thanks for catching should be fixed :)

Comment thread bin/bidsify.py Outdated

def get_first_series(series_list):
"""
For each iterable of BIDSFiles calculate the average series number
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this comment isnt accurate anymore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated!

@jerdra jerdra merged commit c18bbbb into TIGRLab:master Nov 16, 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.

4 participants