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

Split regressor estimation and denoising steps #17

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from
Draft

Conversation

tsalo
Copy link
Collaborator

@tsalo tsalo commented Sep 6, 2024

Closes #10.

Changes proposed in this pull request

  • Collect and run rapidtide on boldref-space BOLD data instead of MNI152NLin6Asym-space data.
    • This should reduce memory use almost all of the time. Plus it makes it easier to warp the lag map to different output spaces.
  • Add option to average lag maps across runs before denoising, which (I think) should reduce noise in the lag maps.
  • Run rapidtide and denoising separately. I think I need to wait until RetroGLM accepts individual files instead of a full rapidtide directory before I can make this work.
  • Add --spcalculation and --noglm flags to Rapidtide interface.
  • Add individual output files as outputs to the Rapidtide interface.

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

Attention: Patch coverage is 32.08556% with 127 lines in your changes missing coverage. Please review.

Project coverage is 30.13%. Comparing base (d4ccf20) to head (22be228).

Files with missing lines Patch % Lines
src/fmripost_rapidtide/workflows/base.py 5.26% 54 Missing ⚠️
src/fmripost_rapidtide/interfaces/rapidtide.py 60.86% 27 Missing ⚠️
src/fmripost_rapidtide/workflows/rapidtide.py 18.75% 26 Missing ⚠️
src/fmripost_rapidtide/interfaces/reportlets.py 31.57% 13 Missing ⚠️
src/fmripost_rapidtide/workflows/confounds.py 16.66% 5 Missing ⚠️
src/fmripost_rapidtide/utils/bids.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   29.03%   30.13%   +1.10%     
==========================================
  Files          25       25              
  Lines        2328     2432     +104     
  Branches      349      276      -73     
==========================================
+ Hits          676      733      +57     
- Misses       1642     1679      +37     
- Partials       10       20      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbfrederick
Copy link
Collaborator

bbfrederick commented Sep 24, 2024

Run rapidtide and denoising separately. I think I need to wait until RetroGLM accepts individual files instead of a full rapidtide directory before I can make this work.

I'm ok with making this change, but why is this needed? If you run rapidtide, even at --outputlevel min, all the files you need to run retroglm will be present in whatever output directory you specified with the proper names, so you can just run retroglm and point it at that work directory. All retroglm requires is that a subset of rapidtide's output files exist in the same directory and follow a naming convention (which is true right after you run rapidtide) - it will ignore any other files in the directory other than what it's looking for. The only reason I can think of to require this is if you run rapidtide, move and rename the output files, and then run retroglm.

EDIT: Ah, I see - you're warping files AFTER running rapidtide and before running retroglm. Still, as long as you follow the naming convention, you can still give a datafileroot argument that will work, but I can see why you'd want flexibility.

@tsalo
Copy link
Collaborator Author

tsalo commented Sep 24, 2024

That's a good point. I can reconstruct a rapidtide-esque directory to feed into RetroGLM.

In that case I think I just need the ability to generate the voxel-wise LFO regressor file in each target space. Can you remind me how to do that without doing any denoising?

@bbfrederick
Copy link
Collaborator

I've added a new script - retrolagtcs to rapidtide v2.9.8 (currently building containers, but passes all its tests).

You give it an fmrifile (for dimensions), the corrmask, processed mask, lag map, and lagtcgenerator, and it will generate voxel specific shifted LFO regressor file (and derivative files, if requested). It currently just reads in the fmrifile, but I just realized I can get all the necessary information just from the header, so I'll do that when I have a moment.

fmrifile, cormask, processed mask, and lag map have to have matching spatial dimensions. The lagtcgenerator can be used unchanged from the initial rapidtide run.

@tsalo
Copy link
Collaborator Author

tsalo commented Sep 24, 2024

Thanks! Just curious, why does it need both the corrmask and the processed mask? Wouldn't just the processed mask be enough?

@bbfrederick
Copy link
Collaborator

bbfrederick commented Sep 24, 2024

To be honest, I just copied that out of retroglm. It probably just needs corrmask (which you could give it twice, I guess). Processed mask is all the voxels you can attempt to fit ("valid voxels"), corrmask is all the places the fit succeeded. In practice they are usually almost identical. I guess the question is, in a nominally valid voxel, where for whatever reason rapidtide could not decide on a delay, is it better to do nothing, or regress out the LFO at zero time delay (because if there is a delay value for that voxel, it's unreliable)? 🤷🏻‍♂️ - I don't feel strongly one way or the other.

EDIT: I suppose I could just pass one mask, and you can decide if you want that to be the corrmask or the processed mask.

@bbfrederick
Copy link
Collaborator

Now it's one mask in 2.9.8.1. I also only read the header of the fmrifile, so it should be lighter on RAM.

@bbfrederick
Copy link
Collaborator

...and now it's 2.9.8.2, because I made a stupid mistake and you can't redeploy to pypi with the same release tag...

@tsalo
Copy link
Collaborator Author

tsalo commented Sep 24, 2024

Thanks so much!

So my plan is basically going to be:

  1. Run rapidtide on boldref-space derivatives to get the mask, lag map, lagtcgenerator, and a few other things.
  2. Average lag maps across runs if users want.
    • To do this, we need to warp the lag maps to a shared space (T1w?), average, and then warp them back
  3. Warp the lag map and mask to desired output spaces.
  4. Run retrolagtcs to get the voxel-wise regressor in each output space.
  5. If users want denoised outputs, run retroglm to actually denoise the BOLD data in those output spaces as well.

@bbfrederick
Copy link
Collaborator

That all looks good, although one thing I don't really know is how much variation in delay maps between runs within a subject is 1) rapidtide fit instability (not real) 2) the effects of other noise processes confounding the fit (not real) and 3) actual physiological variation between runs due to changing circulatory patterns (real). I think number 3 is pretty small - the fact that delay generally seems pretty stable over 5-15 minutes would argue that the delay pattern is pretty fixed, at least during resting state, but I know that Kevin Whittingstall showed that functional activation could cause acute arterial dilation upstream of a strongly activated region, which I think would probably shift the timing between arterial territories. So yes, make averaging between runs an option, but maybe not the default behavior.

@tsalo
Copy link
Collaborator Author

tsalo commented Sep 24, 2024

Oh that's really good to know. I'll make sure it's not the default then. Now I want to dig into that, maybe with HCP-YA.

@bbfrederick
Copy link
Collaborator

We have a paper currently under review looking at repeatability in the HCP-YA data. I can send you a copy if you'd like.

@tsalo
Copy link
Collaborator Author

tsalo commented Sep 24, 2024

That would be great, thanks! Please send to salot at pennmedicine dot upenn dot edu.

@bbfrederick
Copy link
Collaborator

Are you waiting for me to do something on this pull request?

@tsalo
Copy link
Collaborator Author

tsalo commented Oct 29, 2024

No, I just haven't had time to work on fMRIPost-rapidtide for the past couple of weeks. Hopefully I'll be able to carve out some time for it in the next few.

@bbfrederick
Copy link
Collaborator

Ok, no problem. I just didn't want to be holding things up without knowing it.

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.

Rapidtide failing due to memory issue
2 participants