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] Fix long titles overlapping plot contents #3797
Conversation
👋 @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.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
583a346
to
c7ea2a5
Compare
Codecov Report
@@ Coverage Diff @@
## main #3797 +/- ##
=======================================
Coverage 91.53% 91.53%
=======================================
Files 133 133
Lines 15623 15626 +3
Branches 3250 3250
=======================================
+ Hits 14301 14304 +3
Misses 774 774
Partials 548 548
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 |
f6e131a
to
4d6638d
Compare
For review see the artifacts: https://output.circle-artifacts.com/output/job/cde255e7-0a62-4aff-8607-6a8301a138ec/artifacts/0/dev/index.html An example of what to look for where plots are improved: Some other issues like the titles being cut off will be resolved in #3753 or follow-up PRs |
I'm wondering whether it's a good idea to change default size: Wouldn't it be better to work with fontsize or figsize (and do so in some examples to teach this) ? |
Yea it would work to just set figsize in a figure object in the examples. But this way at least guarantees that if you add a title it will fit correctly and without it doesn't look worse (but maybe I missed some cases where it does?). It is always possible to override the figsize in any case. TBH I have no strong opinion one way or the other. Any thoughts @jeromedockes ? |
On second thought, I just went through all the examples and there were a lot of them that were affected by this and they look much better. Going through to manually change each one in the examples code is much more of a hassle. At least this way we have a default that looks nice with or without a title |
I think the proposal in #2804 was to also move the y position of the title, besides increasing the figsize, is this not needed anymore? |
@@ -1816,7 +1818,7 @@ class MosaicSlicer(BaseSlicer): | |||
|
|||
_cut_displayed = "yxz" | |||
_axes_class = CutAxes | |||
_default_figsize = [11.1, 7.2] | |||
_default_figsize = [11.1, 20.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a large increase, why is it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No IIUC that was abandoned for the default figsize approach because increasing the y value will lead to the title getting cut out of the figure, #2804 (comment). I've tested it and it's indeed the case. It seems to me those default x and y position values were chosen to keep the title box "snapped" to the top left corner of the figure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @ymzayek !
Thanks for the reviews! Merging |
Supersedes and closes #2804
Relates to #3728
Closes #2054 .
Changes proposed in this pull request: