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 multi-run workflow synchronized with BIDS dataset #374

Closed
wants to merge 14 commits into from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jan 2, 2021

Closes #217 and supersedes #219.

This is a reduced version of #219, with all extraneous changes removed. Here are the general steps for the proposed workflow:

  1. Load relevant (currently just BOLD) scans from BIDS dataset and determine acquisition time, duration, and generalized name for each.
  2. Extract trigger onsets from BlueprintInput object.
  3. Attempt to match physio trigger onsets to scan onsets.
  4. Interpolate any missing physio trigger periods from available scans.
  5. Identify onsets and offsets for physio trigger periods for each frequency in the physio object.
  6. Slice the physio object into acquisition-specific BlueprintOutputs.
  7. Save BlueprintOutput objects to BIDS files.
  8. Generate a figure showing timing of scans and physio trigger periods before and after synchronization, for QC purposes.

Proposed Changes

  • Add a new module, phys2bids.synchronization, with a function named workflow() meant to perform unsupervised separation of multi-run physio files using onset-time and duration information from a converted BIDS dataset.
  • Add a new function, phys2bids.bids.update_bids_name(), which adds entities to a BIDS-format filename.
  • Add phys2bids.slice4phys.slice_phys(), a function that slices a physio object into individual runs. This is based off of phys2bids.slice4phys.slice4phys(), but is not linked to the existing multi-run workflow.
  • Update package requirements to include scipy, pybids, and pandas.

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@tsalo tsalo added the Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0) label Jan 2, 2021
@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #374 (cab25c7) into master (f587559) will decrease coverage by 17.78%.
The diff coverage is 15.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #374       +/-   ##
===========================================
- Coverage   94.61%   76.83%   -17.79%     
===========================================
  Files           8        9        +1     
  Lines         836     1079      +243     
===========================================
+ Hits          791      829       +38     
- Misses         45      250      +205     
Impacted Files Coverage Δ
phys2bids/synchronization.py 0.00% <0.00%> (ø)
phys2bids/slice4phys.py 40.00% <3.63%> (-57.15%) ⬇️
phys2bids/bids.py 95.80% <90.00%> (-2.26%) ⬇️

@smoia smoia self-assigned this Jan 7, 2021
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

Wow, this is a big one - I didn't notice it in #219 !

Thank you for making the review easier @tsalo, I'd love to see this in action sooner than what we'd have been able to otherwise!

This is not a complete review, I will need much more time to go through the whole code and check everything. However, I did add a couple of comments here and there that I'd ask in a review later on, so if you want you can check them out now and start reply to/address them.

I'll also leave the tests to test-savvy peeps around.

phys2bids/slice4phys.py Outdated Show resolved Hide resolved
phys2bids/slice4phys.py Outdated Show resolved Hide resolved
phys2bids/slice4phys.py Show resolved Hide resolved
phys2bids/synchronization.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@eurunuela eurunuela self-requested a review April 15, 2021 15:08
@@ -290,3 +291,99 @@ def readme_file(outdir):
LGR.warning('phys2bids could not find README,'
'generating it EMPTY, please fill in the necessary info')
utils.write_file(file_path, '', text)


def update_bids_name(basename, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

eh, with nipy/heudiconv#544 I feel that we are doomed to start some minibids or bidsnonofficial or bidspy (so it is not pybids), whatever, to start with a basic schema-driven bids filenames support and may be later expanded to something else (validator etc). otherwise -- we keep breeding duplicated code across the ecosystem, not good! WDYT @tsalo ? I can move that heudiconv PR into its own python package, will you join the fun? ;)

@tsalo
Copy link
Member Author

tsalo commented Aug 12, 2022

@smoia I've realized over time that, not only is FIU's current physio setup suboptimal, but it's also pretty rare. I don't think that many folks would actually find this workflow useful, and I only ended up using it for one project (FIU's now in the process of updating their physio setup). Do you mind if I close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method to split multi-run physio files using BIDS dataset or dicom directory
3 participants