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 reports module #802

Merged
merged 62 commits into from May 31, 2023
Merged

Add reports module #802

merged 62 commits into from May 31, 2023

Conversation

JulioAPeraza
Copy link
Collaborator

Closes #773.

Changes proposed in this pull request:

  • Create HTML files with a summary and figures from a MetaResult object.

@JulioAPeraza JulioAPeraza added the enhancement New feature or request label May 12, 2023
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Patch coverage: 98.27% and project coverage change: +0.50 🎉

Comparison is base (c1e8c31) 87.82% compared to head (0c881cd) 88.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #802      +/-   ##
==========================================
+ Coverage   87.82%   88.33%   +0.50%     
==========================================
  Files          43       46       +3     
  Lines        5643     5931     +288     
==========================================
+ Hits         4956     5239     +283     
- Misses        687      692       +5     
Impacted Files Coverage Δ
nimare/__init__.py 100.00% <ø> (ø)
nimare/diagnostics.py 100.00% <ø> (ø)
nimare/reports/figures.py 97.95% <97.95%> (ø)
nimare/reports/base.py 98.36% <98.36%> (ø)
nimare/reports/__init__.py 100.00% <100.00%> (ø)
nimare/workflows/cbma.py 100.00% <100.00%> (ø)

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

@JulioAPeraza
Copy link
Collaborator Author

It will be difficult to combine view_connectome and the heatmap in the same report.html file. Both plot use plotly, but view_connectome use a fixed plotly.js that is quite old. When I give priority to the latest version view_connectome doesn't render properly, and when I give priority to the old version view_connectome looks fine but the heatmap looks totally wrong (it showed a scatter plot instead of the heatmap).
One solution could be to embed the view_connectome plot to report.html using iframe. Or add a button to open the view_connectome in a separate window.

@JulioAPeraza
Copy link
Collaborator Author

I also noticed that when the heatmap is rendered before view_connectome, both plot looks fine.
This is the error a get when view_connectome is rendered first:

Uncaught TypeError: Plotly.plot is not a function

given that Plotly.plot was deprecated.

@jdkent
Copy link
Member

jdkent commented May 19, 2023

what does the actual html file look like? I'm curious if different versions of the javascript library can be loaded at different sections of the html file.

@JulioAPeraza
Copy link
Collaborator Author

I think this is now ready for review.

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.

Thanks @JulioAPeraza, this is looking awesome! Just want to make sure we aren't working with a combinatorial explosion of parameters when specifying default.yml.

Comment on lines 60 to 62
- bids: {value: z, corr: FDR, method: indep, suffix: fig-summary}
- bids: {value: z, corr: FDR, method: indep, suffix: figure-non}
- bids: {value: z, corr: FDR, method: indep, suffix: figure-interactive}
Copy link
Member

Choose a reason for hiding this comment

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

just so I understand correctly, FDR, 'indep` are just defaults, there is no particular reason why these defaults were chosen, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We chose this combination for the documentation because the FWE montecarlo takes a long time to run with 1000 iterations. The reports function is going to show whatever it finds in the MetaResult object.

subtitle: "Label map: negative tail"
- bids: {value: z, corr: FDR, method: indep, diag: FocusCounter, tab: counts, suffix: figure}
subtitle: FocusCounter
caption: *heatmap_text
Copy link
Member

Choose a reason for hiding this comment

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

do you need to enumerate every potential combination of parameters? what would need to be added here if we were to add a new corrector/diagnostic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you need to enumerate every potential combination of parameters?

Yeah, that's very inefficient, but I couldn't figure out a good way to perform the combinations, like we do with the OS distributions and Python versions during testing.

what would need to be added here if we were to add a new corrector/diagnostic?

We would need to add new bids variables with all their possible combinations.

each cluster in the thresholded map. There is one row for each experiment, and
one column for each cluster, with column names being
<em>PostiveTail</em>/<em>NegativeTail</em> indicating
the sign (+/-) of the cluster's statistical values"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should mention that we re-order the columns and rows so they form clusters in the heatmap

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.

These changes look good to me! Great work, default.yml looks way more maintainable, we will tackle any incompatibilities when we come around to supporting some pairwise estimators and image based meta-analyses.

@jdkent jdkent merged commit 01e28ed into neurostuff:main May 31, 2023
19 checks passed
@JulioAPeraza JulioAPeraza deleted the add-reports branch May 31, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Reports module
2 participants