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] Preventing plot title overlapping content #2804

Closed
wants to merge 3 commits into from

Conversation

OliverWarrington
Copy link

Closes #2054.

Changes proposed in this pull request:

  • Changed the default y position for the title function in BaseSlicer class. Set to 1.08 rather that 0.99, enough to prevent even really long titles overlapping other elements.

Screenshot 2021-05-06 at 10 00 46

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #2804 (78e7f33) into master (034a0bc) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2804      +/-   ##
==========================================
+ Coverage   88.54%   88.56%   +0.02%     
==========================================
  Files         100      100              
  Lines       13392    13392              
  Branches     2608     2608              
==========================================
+ Hits        11858    11861       +3     
+ Misses        953      951       -2     
+ Partials      581      580       -1     
Impacted Files Coverage Δ
nilearn/plotting/displays.py 89.52% <100.00%> (ø)
nilearn/plotting/img_plotting.py 90.14% <0.00%> (ø)
nilearn/reporting/_get_clusters_table.py 100.00% <0.00%> (+2.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 034a0bc...78e7f33. Read the comment docs.

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @OliverWarrington ! 👍
Could you just add a whatsnew entry? (in file doc/whats_new.rst)

@OliverWarrington
Copy link
Author

I think it's done. @NicolasGensollen please could you make sure this is fine? I get a little confused in the updating pull requests world!

@bthirion
Copy link
Member

bthirion commented May 6, 2021

LGTM, but I think we should trigger a full doc rebuild to check that example figs render well...

Copy link
Member

@NicolasGensollen NicolasGensollen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @OliverWarrington !
I just have a very small request (see below).
When you commit this, could you put "[circle full]" somewhere in your commit message? This will trigger a full build on circleCI as mentioned by @bthirion.
If the doc builds correctly, we should be good to merge! 🎉

@@ -7,6 +7,7 @@ NEW
Fixes
-----

- Fix plot title overlapping image content.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the issue number in the description here? It might be hard to know what was fixed otherwise.

@NicolasGensollen
Copy link
Member

Great, thanks!
Let's wait for the CI to finish...

@bthirion
Copy link
Member

bthirion commented May 6, 2021

The problem now is that the title gets cut out of the img on plot_stat_map examples (see CircleCI).

@OliverWarrington
Copy link
Author

The problem now is that the title gets cut out of the img on plot_stat_map examples (see CircleCI).

Ah yep, I see what you mean. I guess the figure size works differently in Jupyter? I'll have another go!

@jeromedockes
Copy link
Member

I think part of the difficulty comes from the fact that the nilearn plotting functions make extensive use of add_axes to place axes in arbitrary, hard-coded positions in the figure as here for example . using this approach rather than positions on grids with GridSpec and add_subplot prevents tight_layout from working and makes the layout more tricky to get right, especially when inputs can vary

@jeromedockes
Copy link
Member

addressing that would be a bigger change than was intended in this PR though, so maybe here if there really isn't enough space you can slightly increase the figure size (the _default_figsize of the *Slicer objects that need it)

@OliverWarrington
Copy link
Author

Thanks @jeromedockes, I'll try increasing the default size. Is there a way to get the same outputs as running CircleCI when I'm checking my changes locally? The outputs of running the plotting examples either in jupyter or just as a script look different to me from the CircleCI outputs.

@NicolasGensollen
Copy link
Member

@OliverWarrington if you want to check the rendering of the docs locally, you can build it by running make html-strict from the doc subfolder. Note that this will build the whole documentation which usually takes around 1h30...
If you have already built it once, you can only rebuild the examples that you modified (see here for more details).

@Remi-Gau Remi-Gau added the Plotting The issue is related to plotting functionalities. label Feb 20, 2023
@ymzayek
Copy link
Member

ymzayek commented Apr 27, 2023

@jeromedockes I was wondering regarding your proposed solution of changing the hardcoded _default_figsize if this could be done in a way that if a title is given a value can be added to the figsize y axis? This would be kind of like what is done with the colorbar in

# Make space for the colorbar
if colorbar:
figsize[0] += .7

Do you think it could work?

@jeromedockes
Copy link
Member

Do you think it could work?

I think so! although it probably isn't a big deal to increase a bit the figure size in all cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Plotting The issue is related to plotting functionalities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot title overlays the image
6 participants