Support clusters table in Diagnostics #765

JulioAPeraza · 2023-02-01T21:14:18Z

Closes None. With this PR we intend to remove reporting.get_clusters_table from the CBMA workflow (#761).

Changes proposed in this pull request:

Jackknife and FocusCounter now return the clusters table in addition to the contribution table.

For a future PR it would be nice to refactor both Jackknife and FocusCounter to reduce some of the duplicated code.

codecov · 2023-02-01T23:34:33Z

Codecov Report

❗ No coverage uploaded for pull request base (main@bd60a9e). Click here to learn what that means.
Patch coverage: 96.77% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #765   +/-   ##
=======================================
  Coverage        ?   88.85%           
=======================================
  Files           ?       39           
  Lines           ?     4405           
  Branches        ?        0           
=======================================
  Hits            ?     3914           
  Misses          ?      491           
  Partials        ?        0

Impacted Files	Coverage Δ
nimare/diagnostics.py	`93.91% <92.85%> (ø)`
nimare/utils.py	`95.53% <98.95%> (ø)`
nimare/workflows/ale.py	`93.42% <100.00%> (ø)`

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tsalo · 2023-02-02T21:19:51Z

Since this changes the numbers of outputs for the two methods, I'd label this PR as a breaking change. Alternatively, you could transpose the diagnostic tables and just tack on the clusters table as additional columns.

This reverts commit d25d8ce.

JulioAPeraza · 2023-02-13T18:23:46Z

nimare/diagnostics.py

@@ -139,7 +150,6 @@ def transform(self, result):
        # Use study IDs in inputs_ instead of dataset, because we don't want to try fitting the
        # estimator to a study that might have been filtered out by the estimator's criteria.
        meta_ids = estimator.inputs_["id"]
-        rows = ["Center of Mass"] + list(meta_ids)

        # Let's label the clusters in the thresholded map so we can use it as a NiftiLabelsMasker


Currently get_clusters_table only return the clusters table, so we need to re-run ndimage.label (reproducing the code in get_clusters_table). There is a current PR in nilearn to return the label map, but that won't be available until the next release.

could you link this pull request in another issue so we can revisit this when nilearn makes another release?

jdkent

LGTM!

tsalo

I think this is really close. Can you just double-check that the ndimage.generate_binary_structure and ndimage.label calls are the same as nilearn? I think if they are, then the cluster labels will always match up between the clusters table and the contributions table.

tsalo · 2023-02-16T21:45:58Z

examples/02_meta-analyses/08_plot_cbma_subtraction_conjunction.py

-knowledge_count_table, _ = counter.transform(knowledge_corrected_results)
+knowledge_count_table, _, _ = counter.transform(knowledge_corrected_results)


Can you show the clusters table in the example?

Should we show the clusters table for both the FocusCounter and Jackknife in this example?

JulioAPeraza · 2023-02-16T23:18:48Z

Thanks, @tsalo!
I think ndimage.generate_binary_structure runs with the same parameters:
nilearn:

# Define array for 6-connectivity, aka NN1 or "faces"
bin_struct = generate_binary_structure(rank=3, connectivity=1)

nimare:

conn = ndimage.generate_binary_structure(rank=3, connectivity=1)

The only difference is in ndimage.label, where nilearn labels positive and negative maps separately and runs on binarized arrays, whereas nimare doesn't make a distinction between positive and negative maps explicitly and runs on the thresholded but non-binarized arrays.
nilearn:

label_map = label(binarized, bin_struct)[0]

nimare:

labeled_cluster_arr, n_clusters = ndimage.label(thresh_arr, conn)

jdkent · 2023-02-21T19:31:28Z

My recommendation is to copy the code from the nilearn pull request and incorporate into nimare until the next nilearn release. Then we can copy how nilearn combines positive and negative clusters into a single table and replicate the order in the contribution tables.

JulioAPeraza · 2023-02-21T23:49:00Z

Sounds good!
I was looking at reporting.get_clusters_table and noticed that for two_sided=True, it lists the clusters with positive values first and then the clusters with negative values. However, the cluster ID is reset, so we will find the first positive and negative clusters sharing the same "Cluster ID"=1.
In the contribution table, we use the cluster ID as the column identifier. Probably, we will need to use a new name for the columns. What do you think?

JulioAPeraza · 2023-02-23T15:10:58Z

Another option would be to split the clusters table into two DFs: one for positive clusters and one for negative clusters. Then, the diagnostic will return three tuples:

return contribution_tables, clusters_tables, labeled_cluster_imgs

where:

contribution_tables = (pos_contribution_table, neg_contribution_table)
clusters_tables = (pos_clusters_table, neg_clusters_table)
labeled_cluster_imgs = (pos_labeled_cluster_img, neg_labeled_cluster_img)

We can check if there are negative values in target_img and run reporting.get_clusters_table with two_sided=True, or add two_sided to the list of parameters in Jackknife and FocusCounter.

JulioAPeraza · 2023-03-02T21:08:57Z

@tsalo @jdkent
I was trying to use get_clusters_tables to get both the clusters table and label maps. We then use the label maps in _transform to get the contribution table.
However, I'm noticing a mismatch in the Cluster ID and the labels in the label_map. This is because clust_ids is sorted based on the peak_vals, but I don't think the label maps are relabeled too: https://github.com/nilearn/nilearn/blob/main/nilearn/reporting/_get_clusters_table.py#L340.

Do you think this is the desired behavior or it is a bug in get_clusters_tables?

tsalo · 2023-03-02T21:27:29Z

I would guess it's a bug. Are you sure that nilearn/nilearn#3477 didn't address the problem?

JulioAPeraza · 2023-03-02T21:41:05Z

I do not think that PR solved the problem. The label_maps wasn't part of the output before, so there was no need to relabel the maps.

If that's the case, perhaps we can solve the issue in our copy of get_clusters_tables by either changing the id (c_id + 1, with c_val) here and here. Or by relabeling the label_map:

re_label_map = np.zeros_like(label_map)
for c_id, c_val in enumerate(clust_ids):
    cluster_mask = label_map == c_val
    re_label_map[cluster_mask] = c_id + 1

What do you think?

into enh-diagnostics

JulioAPeraza · 2023-03-06T17:51:00Z

@tsalo @jdkent, I think this is ready for review. thanks!

jdkent · 2023-03-07T18:35:29Z

nimare/diagnostics.py

+            cluster_ids = sorted(list(np.unique(label_map.get_fdata())[1:]))
+
+            # Create contribution table
+            col_name = "PosTail" if sign == 1 else "NegTail"


I don't have a great suggestion for what the name should be, maybe it should spell out PositiveTail, because it was not immediately clear in the example notebook.

Regardless, If the fact "PosTail represents positive clusters and NegTail represents negative clusters" is added into the documentation of contribution_table then I would feel good about using whatever names you choose.

jdkent · 2023-03-07T18:40:09Z

nimare/utils.py

@@ -1028,6 +1039,161 @@ def unique_rows(ar, return_counts=False):
        return ar[unique_row_indices]


+def _local_max(data, affine, min_distance):


I like to include the permalink of the code I copied:
https://github.com/nilearn/nilearn/blob/8b16d36bbf0f1b88b9ccaea2da1f2867024e16d5/nilearn/reporting/_get_clusters_table.py#L26-L116

or in your case it may be difficult since you have an open pull request to nilearn so the permalink from nilearn does not represent the fix you made for the labels map, so disregard this comment when appropriate

jdkent

LGTM, after our meeting, I think we've settled on decent names/structure of the tables with documentation.

Support clusters table in Diagnostics

862fdbb

JulioAPeraza added the enhancement New feature or request label Feb 1, 2023

Merge branch 'neurostuff:main' into enh-diagnostics

2cf86c5

JulioAPeraza requested review from tsalo and jdkent February 1, 2023 21:16

JulioAPeraza added 2 commits February 1, 2023 16:40

Fix ale workflow

17a4072

Update ale.py

19dce60

JulioAPeraza added the breaking-change PRs that change results or interfaces. label Feb 2, 2023

JulioAPeraza and others added 5 commits February 6, 2023 18:33

Merge branch 'neurostuff:main' into enh-diagnostics

3c5b57a

Use reporting.get_clusters_table to get clusters table

7a808f5

Skip get_clusters_table if table exist in result object

1266dcf

Change order of testing workflows

d25d8ce

Revert "Change order of testing workflows"

d4a4b32

This reverts commit d25d8ce.

JulioAPeraza commented Feb 13, 2023

View reviewed changes

jdkent approved these changes Feb 14, 2023

View reviewed changes

JulioAPeraza mentioned this pull request Feb 14, 2023

Refactor diagnostics module #772

Closed

tsalo requested changes Feb 16, 2023

View reviewed changes

JulioAPeraza added 3 commits February 16, 2023 19:36

Show clusters table in the example

f278d21

Update test_diagnostics.py

4eca1db

Update diagnostics.py

adb1972

Add _get_clusters_table from nilearn

f4411ef

JulioAPeraza and others added 6 commits March 3, 2023 16:37

Update diagnostics.py

9ae4e4a

Merge branch 'neurostuff:main' into enh-diagnostics

76d1c27

Update utils.py

c5e79f4

Merge branch 'enh-diagnostics' of https://github.com/JulioAPeraza/NiMARE

69b85ac

into enh-diagnostics

Update setup.cfg

14c7006

Pin minimum version of nilearn

bd6eec0

jdkent reviewed Mar 7, 2023

View reviewed changes

Update doc, add latest changes from nilearn

809792f

jdkent approved these changes Mar 10, 2023

View reviewed changes

jdkent merged commit c30add5 into neurostuff:main Mar 10, 2023

JulioAPeraza mentioned this pull request Mar 15, 2023

Major refactoring of Diagnostics module #776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support clusters table in Diagnostics #765

Support clusters table in Diagnostics #765

JulioAPeraza commented Feb 1, 2023

codecov bot commented Feb 1, 2023 •

edited

Loading

tsalo commented Feb 2, 2023

JulioAPeraza Feb 13, 2023

jdkent Feb 14, 2023

jdkent left a comment

tsalo left a comment

tsalo Feb 16, 2023

JulioAPeraza Feb 17, 2023

JulioAPeraza commented Feb 16, 2023

jdkent commented Feb 21, 2023

JulioAPeraza commented Feb 21, 2023

JulioAPeraza commented Feb 23, 2023

JulioAPeraza commented Mar 2, 2023

tsalo commented Mar 2, 2023

JulioAPeraza commented Mar 2, 2023 •

edited

Loading

JulioAPeraza commented Mar 6, 2023

jdkent Mar 7, 2023

jdkent Mar 7, 2023

jdkent left a comment

		knowledge_count_table, _ = counter.transform(knowledge_corrected_results)
		knowledge_count_table, _, _ = counter.transform(knowledge_corrected_results)

		@@ -1028,6 +1039,161 @@ def unique_rows(ar, return_counts=False):
		return ar[unique_row_indices]


		def _local_max(data, affine, min_distance):

Support clusters table in Diagnostics #765

Support clusters table in Diagnostics #765

Conversation

JulioAPeraza commented Feb 1, 2023

codecov bot commented Feb 1, 2023 • edited Loading

Codecov Report

tsalo commented Feb 2, 2023

JulioAPeraza Feb 13, 2023

Choose a reason for hiding this comment

jdkent Feb 14, 2023

Choose a reason for hiding this comment

jdkent left a comment

Choose a reason for hiding this comment

tsalo left a comment

Choose a reason for hiding this comment

tsalo Feb 16, 2023

Choose a reason for hiding this comment

JulioAPeraza Feb 17, 2023

Choose a reason for hiding this comment

JulioAPeraza commented Feb 16, 2023

jdkent commented Feb 21, 2023

JulioAPeraza commented Feb 21, 2023

JulioAPeraza commented Feb 23, 2023

JulioAPeraza commented Mar 2, 2023

tsalo commented Mar 2, 2023

JulioAPeraza commented Mar 2, 2023 • edited Loading

JulioAPeraza commented Mar 6, 2023

jdkent Mar 7, 2023

Choose a reason for hiding this comment

jdkent Mar 7, 2023

Choose a reason for hiding this comment

jdkent left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 1, 2023 •

edited

Loading

JulioAPeraza commented Mar 2, 2023 •

edited

Loading