Add Workflow and CBMAWorkflow classes. Support pairwise CBMA workflows #809

JulioAPeraza · 2023-06-02T14:58:15Z

Closes None.

Changes proposed in this pull request:

Convert cbma_workflow function into a class (CBMAWorkflow).
Add Workflow base class.
Support pairwise estimators in Diagnostics.
Support pairwise estimators in Workflow.
Support pairwise estimators in Report.

JulioAPeraza · 2023-06-02T16:38:10Z

@jdkent We are getting an error while building the documentation. It seems related to the nimads source file:

WARNING: /home/docs/checkouts/readthedocs.org/user_builds/nimare/checkouts/809/examples/01_datasets/05_plot_nimads.py failed to execute correctly: Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/nimare/checkouts/809/examples/01_datasets/05_plot_nimads.py", line 28, in <module>
    nimads_studyset = download_file("https://neurostore.org/api/studysets/Cv2LLUqG76W9?nested=true")
  File "/home/docs/checkouts/readthedocs.org/user_builds/nimare/checkouts/809/examples/01_datasets/05_plot_nimads.py", line 25, in download_file
    return response.json()
  File "/home/docs/checkouts/readthedocs.org/user_builds/nimare/envs/809/lib/python3.8/site-packages/requests/models.py", line 975, in json
    raise RequestsJSONDecodeError(e.msg, e.doc, e.pos)
requests.exceptions.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

codecov · 2023-06-02T17:10:25Z

Codecov Report

Patch coverage: 95.73% and project coverage change: +0.02 🎉

Comparison is base (3801b67) 88.37% compared to head (d46e3ca) 88.39%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #809      +/-   ##
==========================================
+ Coverage   88.37%   88.39%   +0.02%     
==========================================
  Files          46       47       +1     
  Lines        5943     6024      +81     
==========================================
+ Hits         5252     5325      +73     
- Misses        691      699       +8

Impacted Files	Coverage Δ
nimare/reports/base.py	`94.92% <83.33%> (-3.44%)`	⬇️
nimare/diagnostics.py	`98.72% <96.22%> (-1.28%)`	⬇️
nimare/reports/figures.py	`98.09% <100.00%> (+0.07%)`	⬆️
nimare/workflows/__init__.py	`100.00% <100.00%> (ø)`
nimare/workflows/ale.py	`95.12% <100.00%> (+0.06%)`	⬆️
nimare/workflows/base.py	`100.00% <100.00%> (ø)`
nimare/workflows/cbma.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

jdkent · 2023-06-02T18:20:57Z

Thanks for the heads up, I should have fixed the issue now.

nimare/diagnostics.py

jdkent

Looking good, just have a couple comments so far.

Could you add a report that showcases the output of a pairwise estimator?
you can use this existing example: 08_plot_cbma_subtraction_conjunction.py

jdkent · 2023-06-07T23:39:16Z

nimare/diagnostics.py

@@ -204,8 +202,14 @@ def transform(self, result):

            contribution_tables.append(contribution_table.reset_index())

-        # Concat PositiveTail and NegativeTail tables
-        contribution_table = pd.concat(contribution_tables, ignore_index=True, sort=False)
+        if pairwaise_estimators or len(label_maps) == 1:


For MKDAChi2, the z-values can be positive or negative (you can check the z-map results from running the test in this pull request: #811).

I think we would want to display both Positive and Negative Tail cluster with Pairwise estimators.

I think we would want to display both Positive and Negative Tail cluster with Pairwise estimators.

We are displaying Positive and Negative tail cluster tables and maps. Here, I'm only excluding the Negative tail contribution table, which will be zero for studies in id1. I think if we want to display that table we would need to plot it in a separate figure and use the id2 for estimating the values. WDYT?

Also in the report, we only show the summary for id1 and coordinates1 from dataset1. Should we include another section for the second group?

This is a little more questionable, overall, I would say yes, we do want to display id2 for the coordinates. for the contribution table, this is a bit debatable. With ALESubtraction, yes, I think it is useful to know how both groups contributed to the existing clusters.

For the MKDAChi2 I think it is less useful to see a contribution table (and with jacknife it would be computationally prohibitive if one was using the entire neurosynth dataset).

But the notion that ALESubtraction is used for more similarly sized groups and MKDAChi2 is used for comparing a relatively smaller dataset to a huge dataset is only reflective of how we are currently the algorithms, there's nothing stopping you from using ALESubtraction with neurosynth.

So I think the answer would be creating an option/flag for the user to decide whether they want to show the second group or not. could be called display_second_group or something, with a default value of False.

jdkent · 2023-06-07T23:46:33Z

nimare/workflows/__init__.py

-__all__ = ["ale_sleuth_workflow", "cbma_workflow", "macm_workflow"]
+__all__ = [
+    "ale_sleuth_workflow",
+    "Workflow",


I would remove the Workflow baseclass from __all__, this is not a public facing class (e.g., do not want users importing and trying to use it by mistake.

… rtd

…tion

…estimator

jdkent

Thanks for all your work on this!, I have a couple comments/questions for idea on how to restructure small sections of code.

jdkent · 2023-06-16T01:10:41Z

nimare/diagnostics.py

@@ -274,9 +295,12 @@ def _transform(self, expid, label_map, result):
        # with one missing a study in its inputs.
        estimator = copy.deepcopy(result.estimator)

-        dset = estimator.dataset
+        if self._is_pairwaise_estimator:
+            all_ids = estimator.inputs_["id1"]


would all ids be id1 and id2, the test case in the test suite is an unusual scenerio where the same dataset is used for id1 and id2.

jdkent · 2023-06-16T01:12:31Z

nimare/diagnostics.py

+            temp_dset = (
+                estimator.dataset1.slice(other_ids)
+                if sign == "PositiveTail"
+                else estimator.dataset2.slice(other_ids)


for "NegativeTail" is the temp result fitting dataset2 (minus one analysis) and dataset2, but should be dataset1, dataset2 (minus one analysis)

Good catch! I forgot to cover these cases.

jdkent · 2023-06-16T03:12:07Z

nimare/diagnostics.py

+        elif "stat_desc-group1MinusGroup2" in result.maps:
+            target_value_map = "stat_desc-group1MinusGroup2"
+        elif "z_desc-specificity" in result.maps:
+            target_value_map = "z_desc-specificity"
        else:
            target_value_map = "z"


this could be replaced by taking the union of two sets? one set being ("est", "stat", "stat_desc-group1MinusGroup2", "z_desc-specificity") and the other being the result.maps, then if the union of the sets is empty, have the target_value_map be "z".

Or taking a step back, the purpose of this is to find the uncorrected values, correct? (if the target_image was the corrected values, so one could theoretically manipulate the target_image name to create the target_value_map name, then it would just be a parameter instead of a list of if statements.

this could be replaced by taking the union of two sets?

Good idea!

if the target_image was the corrected values, so one could theoretically manipulate the target_image name to create the target_value_map name, then it would just be a parameter instead of a list of if statements.

That's what I initially tried to do, but I think the logic here is to use stat maps if possible, if they are not available then use z maps.

jdkent

I think "PositiveTail" and "NegativeTail" should be defined as variables and described since they are written as strings a few too many times, so it could be hard to get the context of what the string means and more difficult to change in the future if we have to change every string instead of a variable.

JulioAPeraza · 2023-06-21T20:24:11Z

Great point.

jdkent

sorry, just another minor comment, I think after that it looks good to me.

jdkent · 2023-06-22T02:13:27Z

examples/02_meta-analyses/08_plot_cbma_subtraction_conjunction.py

@@ -129,7 +131,7 @@
 )
 related_corrected_results = jackknife.transform(related_corrected_results)


wasn't from this pull request, but I think this variable should have a different name, like related_diagnostic_results, so that related_corrected_results is not overwritten. follows the convention of going from regular results to corrected results.

jdkent

LGTM! Thanks @JulioAPeraza

JulioAPeraza added 2 commits May 31, 2023 18:51

Convert cbma_workflow into a class. Support pairwise estimators

cb1fa66

Add support for reports and diagnostics with pairwise estimator

72e1ef4

JulioAPeraza added the enhancement New feature or request label Jun 2, 2023

JulioAPeraza added 5 commits June 2, 2023 11:52

fix dicstring

adfb9f1

add deprecation warning

48b5aed

fix test for tables == None

5ffa968

add version changes to docstring

7a40379

fix documentation

73fc15e

improve coverage

8839cf6

jdkent reviewed Jun 7, 2023

View reviewed changes

nimare/diagnostics.py Outdated Show resolved Hide resolved

JulioAPeraza added 2 commits June 7, 2023 16:38

Use specificity maps instead

9b77e9b

Merge _preprocess_input

9fb98cc

jdkent reviewed Jun 7, 2023

View reviewed changes

JulioAPeraza and others added 14 commits June 8, 2023 14:51

remove Workflow from __init__

5455231

Add Pairwise estimator report

ebd4b13

update diagnostics

56ec484

reduce the number of iterations. we are running out of time/memory in…

78bcb0e

… rtd

Merge branch 'neurostuff:main' into enh-workflow

1d25b02

see if using focuscounter reduces the time for building the documenta…

2268f17

…tion

New parameter display_second_group

c7a7bf8

Update 08_plot_cbma_subtraction_conjunction.py

b08e712

Add dataset 2 to summary

a3573af

Update diagnostics.py

6c50469

Update versionchanged

3447502

Merge branch 'neurostuff:main' into enh-workflow

5d33e92

Reorder matrix only if more than 1 cluster/experiment

5630c9f

display_second_group in the example

4f18d84

JulioAPeraza mentioned this pull request Jun 13, 2023

plotly graph for reports has large text when report viewed locally #814

Closed

JulioAPeraza and others added 6 commits June 13, 2023 10:32

fix neurostuff#814

98bc738

Use iframe only for connectome

9b950e3

Set the size of the heatmap proportional to rows and columns

363effd

Separate positive from negative tail contribution table for pairwise …

fa5d876

…estimator

Add subsubtitle

385091d

Merge branch 'main' into enh-workflow

761ee12

jdkent requested changes Jun 16, 2023

View reviewed changes

JulioAPeraza added 3 commits June 16, 2023 12:43

Test a realistic scenario with different dset1 and desert 2

b923cd9

Apply @jdkent code review

73347ad

consider the length of the study label in the figure size

e7360ed

JulioAPeraza marked this pull request as draft June 21, 2023 16:06

jdkent requested changes Jun 21, 2023

View reviewed changes

fix issues with figure sizes

afdf4a4

Define "PositiveTail" and "NegativeTail" as variables

fbd7ae4

JulioAPeraza marked this pull request as ready for review June 21, 2023 20:24

Update diagnostics.py

a4e8acd

JulioAPeraza marked this pull request as draft June 21, 2023 22:11

Make a distinction between studies and experiments in report

883fa6e

JulioAPeraza marked this pull request as ready for review June 21, 2023 22:34

JulioAPeraza added 2 commits June 21, 2023 18:57

Restore the diagnostics summary

cc1a3e0

Limit the colormap to the total number of clusters

8ac8e50

jdkent requested changes Jun 22, 2023

View reviewed changes

Update 08_plot_cbma_subtraction_conjunction.py

d46e3ca

jdkent approved these changes Jun 22, 2023

View reviewed changes

jdkent merged commit c4346a2 into neurostuff:main Jun 22, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Workflow and CBMAWorkflow classes. Support pairwise CBMA workflows #809

Add Workflow and CBMAWorkflow classes. Support pairwise CBMA workflows #809

JulioAPeraza commented Jun 2, 2023

JulioAPeraza commented Jun 2, 2023

codecov bot commented Jun 2, 2023 •

edited

Loading

jdkent commented Jun 2, 2023

jdkent left a comment

jdkent Jun 7, 2023

JulioAPeraza Jun 8, 2023

JulioAPeraza Jun 8, 2023

jdkent Jun 12, 2023

jdkent Jun 7, 2023

jdkent left a comment

jdkent Jun 16, 2023

jdkent Jun 16, 2023

JulioAPeraza Jun 16, 2023

jdkent Jun 16, 2023

JulioAPeraza Jun 16, 2023 •

edited

Loading

jdkent left a comment

JulioAPeraza commented Jun 21, 2023

jdkent left a comment

jdkent Jun 22, 2023

jdkent left a comment

		@@ -129,7 +131,7 @@
		)
		related_corrected_results = jackknife.transform(related_corrected_results)

Add Workflow and CBMAWorkflow classes. Support pairwise CBMA workflows #809

Add Workflow and CBMAWorkflow classes. Support pairwise CBMA workflows #809

Conversation

JulioAPeraza commented Jun 2, 2023

JulioAPeraza commented Jun 2, 2023

codecov bot commented Jun 2, 2023 • edited Loading

Codecov Report

jdkent commented Jun 2, 2023

jdkent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdkent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulioAPeraza Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

jdkent left a comment

Choose a reason for hiding this comment

JulioAPeraza commented Jun 21, 2023

jdkent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdkent left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2023 •

edited

Loading

JulioAPeraza Jun 16, 2023 •

edited

Loading