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.

Sign up for GitHub

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

Jump to bottom

[DOC] Fix rendering of plots in some examples where titles or images are cut off #3753

Merged
merged 12 commits into from Jul 14, 2023
Merged

[DOC] Fix rendering of plots in some examples where titles or images are cut off #3753

merged 12 commits into from Jul 14, 2023

Conversation

ymzayek
Copy link
Member

@ymzayek ymzayek commented Jun 14, 2023

Handles part of #3728

@github-actions
Copy link
Contributor

👋 @ymzayek 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.

@codecov
Copy link

codecov bot commented Jun 14, 2023 •
edited

Codecov Report

Merging #3753 (e2d5e5c) into main (91218eb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3753   +/-   ##
=======================================
  Coverage   91.59%   91.59%           
=======================================
  Files         134      134           
  Lines       15694    15694           
  Branches     3270     3270           
=======================================
  Hits        14375    14375           
  Misses        770      770           
  Partials      549      549
Flag Coverage Δ
macos-latest_3.10 91.51% <ø> (ø)
macos-latest_3.11 91.51% <ø> (ø)
macos-latest_3.8 91.48% <ø> (ø)
macos-latest_3.9 91.48% <ø> (ø)
ubuntu-latest_3.10 91.51% <ø> (ø)
ubuntu-latest_3.11 91.51% <ø> (ø)
ubuntu-latest_3.8 91.48% <ø> (ø)
ubuntu-latest_3.9 91.48% <ø> (ø)
windows-latest_3.10 91.46% <ø> (ø)
windows-latest_3.11 91.46% <ø> (ø)
windows-latest_3.8 91.42% <ø> (ø)
windows-latest_3.9 91.42% <ø> (ø)

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

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bthirion
Copy link
Member

LMK whenever you want me to have a look on it.

@Remi-Gau Remi-Gau linked an issue Jun 19, 2023 that may be closed by this pull request
[DOC] Update and improve examples in nilearn documentation #3728
Closed
11 tasks
@ymzayek ymzayek mentioned this pull request Jul 5, 2023
Merged
@ymzayek ymzayek marked this pull request as ready for review July 13, 2023 08:59
@ymzayek
Copy link
Member Author

ymzayek commented Jul 13, 2023

The best way to review this is the doc build artifacts. Here is a link to the examples: https://output.circle-artifacts.com/output/job/caf24ad1-f374-48bc-9e74-1f3d185d7fbc/artifacts/0/dev/auto_examples/index.html
The ones where a thumbnail from an example is rendered are the ones that are affected by this PR.

Note that this PR does not correct the overlapping titles of matrix plots. I will do one more PR to fix those.

Remi-Gau
Remi-Gau reviewed Jul 13, 2023
View reviewed changes
examples/01_plotting/plot_demo_more_plotting.py Outdated
@@ -43,6 +43,8 @@
# First, we retrieve data from nilearn provided (general-purpose) datasets
# ------------------------------------------------------------------------

import matplotlib.pyplot as plt
Copy link
Collaborator

Choose a reason for hiding this comment

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

out of curiosity: why is this new import needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's used to change the figure size in the modified plots in L122 and L134. Maybe better to move it below to where it's used

Remi-Gau
Remi-Gau approved these changes Jul 13, 2023
View reviewed changes
Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

Good with me !! 🚀

bthirion
bthirion approved these changes Jul 14, 2023
View reviewed changes
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, thx.

@Remi-Gau
Copy link
Collaborator

Good to go!! Merging.
thanks @ymzayek

@Remi-Gau Remi-Gau merged commit e23baad into nilearn:main Jul 14, 2023
29 checks passed
@ymzayek ymzayek deleted the cutoff-titles branch July 17, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bthirion bthirion bthirion approved these changes

@Remi-Gau Remi-Gau Remi-Gau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DOC] Update and improve examples in nilearn documentation
3 participants
@ymzayek @bthirion @Remi-Gau