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 CBMA workflow #761

Merged
merged 36 commits into from Mar 30, 2023
Merged

Add CBMA workflow #761

merged 36 commits into from Mar 30, 2023

Conversation

JulioAPeraza
Copy link
Collaborator

Closes #760.

Changes proposed in this pull request:

  • Add CBMA workflow: estimator, with corrector and diagnostics.
  • Fix a typo in MetaResult.save_tables().

@JulioAPeraza JulioAPeraza added the enhancement New feature or request label Jan 23, 2023
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.40 🎉

Comparison is base (8c377e1) 88.46% compared to head (6af419b) 88.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #761      +/-   ##
==========================================
+ Coverage   88.46%   88.86%   +0.40%     
==========================================
  Files          40       41       +1     
  Lines        4758     4823      +65     
==========================================
+ Hits         4209     4286      +77     
+ Misses        549      537      -12     
Impacted Files Coverage Δ
nimare/diagnostics.py 99.13% <100.00%> (ø)
nimare/meta/cbma/base.py 96.10% <100.00%> (ø)
nimare/results.py 85.33% <100.00%> (+15.47%) ⬆️
nimare/workflows/__init__.py 100.00% <100.00%> (ø)
nimare/workflows/cbma.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tsalo tsalo added the workflows Issues related to the workflows module label Jan 23, 2023
nimare/workflows/cbma.py Outdated Show resolved Hide resolved
nimare/workflows/cbma.py Outdated Show resolved Hide resolved
Copy link
Member

@jdkent jdkent left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple changes for logging messages

@jdkent
Copy link
Member

jdkent commented Mar 14, 2023

I think a notebook example could be helpful for this pull request, I started one here: https://github.com/jdkent/NiMARE/blob/cbma-workflow/examples/02_meta-analyses/10_plot_cbma_workflow.py

@jdkent
Copy link
Member

jdkent commented Mar 14, 2023

the boilerplate pull request was merged in, so the items for this pull request are:

  1. support string inputs ("ale", "mkda", "fwe", "fdr", "jackknife", etc.) for novice users so they do not need to import (include the options in the docstring of the class)
  2. change the default corrector to FWE montecarlo
  3. refactor diagnostics to have a base class and add a "return_clusters_table_only" flag (perhaps with another name) that only returns the cluster table to account for the case where the estimator is not compatible with the diagnostics.
  4. look at the ALE workflow to see how that workflow incorporates the boilerplate and do the same here
  5. create an example (in the gallery) demonstrating the usage of the workflow.

@JulioAPeraza
Copy link
Collaborator Author

change the default corrector to FWE montecarlo

One potential issue with setting the default corrector to FWE montecarlo is that the example with a single line result = cbma_workflow(dset) will run 10000 iterations which may impact the running time of the example gallery. WDYT?

@jdkent
Copy link
Member

jdkent commented Mar 16, 2023

lets bring this up in the meeting, perhaps FDR could be a better default.

Copy link
Member

@jdkent jdkent left a comment

Choose a reason for hiding this comment

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

This is looking good!

Just a few minor comments from me.

nimare/workflows/cbma.py Outdated Show resolved Hide resolved
nimare/workflows/cbma.py Outdated Show resolved Hide resolved
nimare/workflows/cbma.py Outdated Show resolved Hide resolved
nimare/workflows/cbma.py Outdated Show resolved Hide resolved
nimare/workflows/cbma.py Show resolved Hide resolved
Co-authored-by: James Kent <jamesdkent21@gmail.com>
@jdkent
Copy link
Member

jdkent commented Mar 21, 2023

@JulioAPeraza
Copy link
Collaborator Author

for FDR/Bonferonni can we use nilean instead?

@jdkent @tsalo I was trying to implement this, but I'm a little confused about a couple of things.

  1. If we adopt threshold_stats_img for FDR and FWE Bonferroni, should we also return thresholded maps from FWE Montecarlo? The Montecarlo method removes clusters that are not significant, but it doesn't actually threshold the maps. We can still see non-significant voxels in significant clusters.
  2. Also, by using a threshold_stats_img for FDR and Bonferroni we won't be able to return a corrected p-values map as we currently support (since threshold_stats_img only work on z maps), and also the correct_fdr_negcorr method won't be supported.
  3. Another difference is in the z values itself. With the current behavior, the Corrector return corrected z values, but with threshold_stats_img the z values will be thresholded but not corrected. I was wondering if this would be Ok.

An alternative approach would be to add an alpha parameter to the cbma_workflow to use in the diagnostics. That parameter will be used only for visualization and determining the clusters for the contribution/clusters table. Alpha will be independent of the cluster-forming threshold that is used in the FWE Montecarlo.
WDYT?

@jdkent
Copy link
Member

jdkent commented Mar 23, 2023

oof, I mean I still like the idea of off-loading this functionality to nilearn, but perhaps in the interim we do something similar to how we are handling cluster tables? Copy their code and modify it so it does not return thresholded maps?

I can see the argument from a data sharing perspective that we don't want to arbitrarily "zero-out" voxels. that's useful information generally.

I was viewing it through a result presentation perspective, where if you correct the results at a particular threshold, then that is the threshold that should be applied to the data.

@jdkent
Copy link
Member

jdkent commented Mar 23, 2023

but perhaps the nilearn implementation is too different for our use case.

@jdkent
Copy link
Member

jdkent commented Mar 30, 2023

everything looks good to me, great work @JulioAPeraza!

@jdkent
Copy link
Member

jdkent commented Mar 30, 2023

For now the nilearn method of thresholding is a different concept of how we are doing correction.

Copy link
Member

@jdkent jdkent left a comment

Choose a reason for hiding this comment

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

LGTM!

@jdkent jdkent merged commit 5088944 into neurostuff:main Mar 30, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request workflows Issues related to the workflows module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run an arbitrary CBMA workflow
3 participants