bbfrederick / rapidtide

rapidtide - a suite of programs for doing time lag correlation analysis on fMRI data
Apache License 2.0
75 stars 14 forks source link

Update fMRIPrep-related documentation #172

Closed tsalo closed 1 month ago

tsalo commented 1 month ago

Closes none. I'm trying to translate the settings we're planning to use in fMRIPost-rapidtide into recommendations in rapidtide's documentation.

Changes proposed in this pull request:

bbfrederick commented 1 month ago

All good suggestions and changes. The wording about extracting transforms from the working directory is from the bad old days before these were output by default.

I'm leaning towards using the gray matter mask for globalmeaninclude as well. In fact, along those lines, there is a new capability in the last few versions that I haven't documented yet - instead of supplying the masks individually, you can just give rapitdide --brainmask, --graymattermask and --whitemattermask, and it sets the internal masks based on whatever current theory I have about what regions are good to use for each one (if you explicitly set any of the masks on the command line it overrides the "standard" behavior for that mask.

bbfrederick commented 1 month ago

I'm not sure what's making build_docs fail - sphinx is a bit of a black box to me; the documentation broke a while back and I threw code at the wall until it started working again then ran way - I'm afraid to touch it now :-).

tsalo commented 1 month ago

I have been using those three parameters when processing 28andMe. I'm happy to modify the fMRIPrep recommendations to use those parameters instead. If you use --graymattermask, should you still use the same mask for --refineinclude as well? For healthy young adults, I mean.

Also... since these mask files are binarized automatically with a threshold of 0.1, do you still recommend binarizing fMRIPrep probseg files before using them?

The two problems I stumbled across are that the intersphinx mapping dictionary was formatted in a way that isn't supported anymore and m2r recently started breaking. m2r has been unmaintained for a while, so I believe the current recommendation is to use myst_parser, which is what I'm trying.

bbfrederick commented 1 month ago

I don't have any firm thoughts about what to use for --refineinclude. On the one hand, gray matter is likely to have the strongest signal, so you might get a higher quality regressor, but including white matter makes it more likely that you'll get a regressor that's more "balanced" to all tissue types (then again, if it should be the same regressor everywhere, the gray matter signal should be sufficient). For the time being, I think I'll leave --graymattermask as is - it will set --globalmeaninclude and --offsetinclude but not do anything to --refineinclude, unless you feel strongly that it should set --refineinclude too.

tsalo commented 1 month ago

I definitely don't have strong feelings about it. For the fMRIPrep recommendations, I can just not make one regarding refineinclude.

tsalo commented 1 month ago

Okay, I think the recommendations are fairly solid/up-to-date now. What do you think?

bbfrederick commented 1 month ago

Looks good. I'll merge it.

bbfrederick commented 1 month ago

Actually, NOW I'll merge it.

tsalo commented 1 month ago

Thanks! Although it looks like you closed without merging.

bbfrederick commented 1 month ago

That explains a lot. But why is build_docs failing again?

tsalo commented 1 month ago

I think that's just a dumb mistake on my part. I just pushed a fix, but you should wait to make sure it works before merging.

tsalo commented 1 month ago

Argh, the changelog page is being rendered (i.e., the URL exists), but it's not showing up in the sidebar.

bbfrederick commented 1 month ago

build_docs passed, and I have no reason to think the coverage test will fail (since no code changed). Merge?

tsalo commented 1 month ago

I just need to fix the changelog. I think I know what's wrong though.

bbfrederick commented 1 month ago

Ok. Are we .md free going forward?

tsalo commented 1 month ago

Not quite. It's actually basically the same as before, where whatsnew.rst just collects the contents of CHANGELOG.md, but using myst_parser instead of m2r or recommonmark, since both of the latter are unmaintained at this point.

tsalo commented 1 month ago

The sidebar looks good now! image

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 34.56%. Comparing base (c598b78) to head (4058e26). Report is 34 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #172 +/- ## ========================================== + Coverage 34.45% 34.56% +0.10% ========================================== Files 135 135 Lines 24334 24334 Branches 3725 3501 -224 ========================================== + Hits 8385 8411 +26 + Misses 15129 15128 -1 + Partials 820 795 -25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.