Skip to content

WIP: Add FFX/RFX estimators #163

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

Closed
wants to merge 20 commits into from
Closed

WIP: Add FFX/RFX estimators #163

wants to merge 20 commits into from

Conversation

tyarkoni
Copy link
Contributor

@tyarkoni tyarkoni commented Aug 2, 2019

This PR adds new FFX and RFX estimators and restructures some of the estimation code. Main changes:

  • A new ffx estimation function that runs a standard fixed-effects meta-analysis.
  • A method argument to the FFX class (renamed from FFX_GLM) that allows users to specify 'fsl' optionally (but defaults to the standard approach).
  • Removed the ffx_glm and mfx_glm functions as they were just wrappers for fsl_glm and we now have (or soon will have) alternative methods for FFX and RFX.

TO DO:

  • Add mixed-effect estimation to stan_mfx
  • Add a StanMFX class that wraps stan_mfx and follows the existing API
  • Split stan_mfx into separate compilation and estimation steps for efficiency
  • Get PyStan to install correctly on CircleCI
  • Improve returned results (to include credible intervals, pseudo-p values, etc.)
  • Auto-scale priors based on data to avoid initialization failures and improve sampling efficiency

@tyarkoni
Copy link
Contributor Author

tyarkoni commented Aug 2, 2019

@tsalo: I'm now assuming var_maps instead of se_maps, and haven't touched the code in fsl_glm. Presumably that needs to be updated to make sure that we compute se_maps
from the var_maps and sample_sizes.

@tsalo
Copy link
Member

tsalo commented Aug 2, 2019

Per a conversation with Camille Maumet, here, we don't scale the se or var maps by sample size. We just square the se maps to make the flameo-compatible var maps. All we should have to do is remove the line squaring the maps.

@tyarkoni
Copy link
Contributor Author

tyarkoni commented Aug 2, 2019

Are we talking about standard error maps, or standard deviation maps? Standard errors are estimates of the SD of the population distribution underlying the observed sample. They depend on the sample size (because the estimate gets more precise the larger the sample). But if FSL expects variance maps, then yes, we just need to drop the sqrt.

@nicholst
Copy link

nicholst commented Aug 3, 2019 via email

@codecov
Copy link

codecov bot commented Aug 14, 2019

Codecov Report

Merging #163 into master will decrease coverage by 0.12%.
The diff coverage is 17.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
- Coverage   45.83%   45.71%   -0.13%     
==========================================
  Files          68       68              
  Lines        4038     4047       +9     
==========================================
- Hits         1851     1850       -1     
- Misses       2187     2197      +10
Impacted Files Coverage Δ
nimare/meta/ibma.py 31.01% <30%> (-0.4%) ⬇️
nimare/meta/esma.py 83.46% <7.69%> (-8.65%) ⬇️

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 43eb19e...bfd42a6. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 14, 2019

Codecov Report

Merging #163 into master will increase coverage by <.01%.
The diff coverage is 43.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   45.45%   45.45%   +<.01%     
==========================================
  Files          70       70              
  Lines        4380     4448      +68     
==========================================
+ Hits         1991     2022      +31     
- Misses       2389     2426      +37
Impacted Files Coverage Δ
nimare/info.py 0% <ø> (ø) ⬆️
nimare/meta/esma.py 73.17% <28.57%> (-19.01%) ⬇️
nimare/meta/ibma.py 31.01% <30%> (-0.4%) ⬇️
nimare/tests/test_esma.py 93.1% <66.66%> (-6.9%) ⬇️
nimare/tests/utils.py 90.74% <88.23%> (-1.16%) ⬇️
nimare/tests/conftest.py 98.66% <0%> (ø) ⬆️

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 1ffd423...5fb18f1. Read the comment docs.

@tyarkoni
Copy link
Contributor Author

Still a WIP, but the stan_mfx function should be working, and is fairly flexible. I decided to go with an MCMC implementation to start with, as it's way easier to implement, more flexible, and is likely to be less biased. The obvious cost is computation time, but it's not as bad as I expected—a few hundred models (e.g., for an atlas-based reduction) shouldn't take more than an hour or two on most systems, and even whole-brain analysis should be feasible on a cluster (though we'll probably need to write code that distributes different voxels to different workers to really make that work well).

@tyarkoni
Copy link
Contributor Author

tyarkoni commented Aug 17, 2019

I've generalized stan_mfx so that it can now do mega-analysis as well as meta-analysis. Basically you can pass various combination of estimates, variances, standard_errors, and groups, and it will generally build you the best possible model given what you passed in. Once we merge this and #173, we'll be in a position to do ROI-level Bayesian mixed-effects meta/mega-analysis estimation fairly efficiently... pretty exciting!

@tsalo
Copy link
Member

tsalo commented Jul 20, 2020

Now that PyMARE is up and running, and the PyMARE integration is mostly handled (minus Stan and MCC), should we close this?

@tyarkoni tyarkoni closed this Nov 3, 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.

3 participants