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

FIX] fixes several plotting details in glm reports #4266

Merged
merged 8 commits into from Feb 29, 2024

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Feb 13, 2024

Changes proposed in this pull request:

  • only display FIR delay if FIR is used
  • make sure the run number does not overlap with the design matrix also only display it if there is more then one run
  • change number of significant figure in parameters for thresholding to make sure default alpha is properly displayed and use scientific notation for very small alphas
  • add "z-score" next to color bar
    • handle black background
  • for plot_type="slice"
    • use symmetric blue to white colormap when data has both positive and negative values
    • use "red_transparent_full_alpha_range" colormap when data only has positive values
    • use "blue_transparent_full_alpha_range" colormap when data only has negative values
  • use a proper color map for glass brain

Copy link
Contributor

👋 @Remi-Gau Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

  • PR has an interpretable title.
  • PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
  • Code is PEP8-compliant (see our documentation on coding style)
  • Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

  • There is at least one unit test per new function / class (see our documentation on testing)
  • The new feature is demoed in at least one relevant example.

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

@Remi-Gau
Copy link
Collaborator Author

Run number next to design matrices

image

@Remi-Gau
Copy link
Collaborator Author

Add unit to color bar

image

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: Patch coverage is 62.96296% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 92.04%. Comparing base (abb80ff) to head (7ed968a).
Report is 16 commits behind head on main.

Files Patch % Lines
nilearn/reporting/glm_reporter.py 62.96% 6 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4266      +/-   ##
==========================================
+ Coverage   91.85%   92.04%   +0.19%     
==========================================
  Files         144      143       -1     
  Lines       16419    16438      +19     
  Branches     3434     3445      +11     
==========================================
+ Hits        15082    15131      +49     
+ Misses        792      764      -28     
+ Partials      545      543       -2     
Flag Coverage Δ
macos-latest_3.10_test_plotting 91.92% <62.96%> (?)
macos-latest_3.11_test_plotting 91.92% <62.96%> (+0.06%) ⬆️
macos-latest_3.12_test_plotting 91.92% <62.96%> (?)
macos-latest_3.9_test_plotting 91.88% <62.96%> (?)
ubuntu-latest_3.10_test_plotting 91.92% <62.96%> (+0.06%) ⬆️
ubuntu-latest_3.11_test_plotting 91.92% <62.96%> (?)
ubuntu-latest_3.12_test_plotting 91.92% <62.96%> (?)
ubuntu-latest_3.12_test_pre 91.92% <62.96%> (?)
ubuntu-latest_3.8_test_plot_min 91.63% <62.96%> (?)
ubuntu-latest_3.8_test_plotting 91.88% <62.96%> (?)
ubuntu-latest_3.9_test_plotting 91.88% <62.96%> (?)
windows-latest_3.10_test_plotting 91.89% <62.96%> (?)
windows-latest_3.11_test_plotting 91.89% <62.96%> (?)
windows-latest_3.12_test_plotting 91.89% <62.96%> (?)
windows-latest_3.8_test_plotting 91.85% <62.96%> (?)
windows-latest_3.9_test_plotting 91.86% <62.96%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Remi-Gau
Copy link
Collaborator Author

Note: consider doing the colormap modifications in a separate PR as the default choice may require a proper discussion.

@Remi-Gau
Copy link
Collaborator Author

Note: review and visualizing the outcome would be easier if #2194 (comment) was addressed first

@Remi-Gau
Copy link
Collaborator Author

@Remi-Gau Remi-Gau removed the Blocked label Feb 28, 2024
@Remi-Gau Remi-Gau marked this pull request as ready for review February 28, 2024 09:28
@Remi-Gau Remi-Gau changed the title [WIP][FIX] fixes several plotting details in glm reports FIX] fixes several plotting details in glm reports Feb 28, 2024
@Remi-Gau
Copy link
Collaborator Author

Requested a full build to compare to the GLM reports before and after.

Will post the links once the builds are done.

Copy link
Member

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

LGTM.

@Remi-Gau Remi-Gau merged commit 1afacef into nilearn:main Feb 29, 2024
32 of 33 checks passed
@Remi-Gau Remi-Gau deleted the 2405 branch February 29, 2024 07:48
Remi-Gau added a commit to Remi-Gau/nilearn that referenced this pull request Mar 5, 2024
* fix several issues in glm reports

* further fixes

* change cmap for glass brain

* fix remaining issues

* [full doc] request full build

* update changelog

* [full doc] request full build
Remi-Gau added a commit that referenced this pull request Mar 9, 2024
…ap with masker report images (#4308)

* extract table folding and overlapping

* update changelog

* Update doc/changes/latest.rst

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

* FIX] fixes several plotting details in glm reports (#4266)

* fix several issues in glm reports

* further fixes

* change cmap for glass brain

* fix remaining issues

* [full doc] request full build

* update changelog

* [full doc] request full build

* [FIX] force views in surface plotting to be pairs of `int` or `float` when not a `string` (#4297)

* for views to be pairs of int or float when not a string

* update changelog

* [FIX] fix cut position in nifti maps maskers (#4304)

* fix cut position in nifti maps maskers

* update changelog

* [FIX] remove conftest for externals (#4306)

* remove conftest tempita

* ignore tempita in pyproject.toml

* [MAINT] add script to check duration of test suite (#4231)

* add script to check duration of test suite

* add comment

* comment and refactor

* typo

* improve paging

* add workflow

* Update maint_tools/requirements.txt

* fail on bad request

* upload artefacts

* upload artefacts

* remove running on PR

* Apply suggestions from code review

Co-authored-by: Elizabeth DuPre <emd222@cornell.edu>

* update doc string

* update doc string

* refactor

---------

Co-authored-by: Elizabeth DuPre <emd222@cornell.edu>

* [DOC] display all masker reports in the doc (#4295)

* display masker reports in doc

* update changelog

* refactor

* add notebooks

* clear cells

* [full doc] request full build

* add link to examples

* [full doc] request full build

* fix doc build

* [full doc] request full build

* [full doc] request full build

---------

Co-authored-by: bthirion <bertrand.thirion@inria.fr>
Co-authored-by: Elizabeth DuPre <emd222@cornell.edu>
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.

Misc issues in Nistats reports
2 participants