Skip to content

FIX: tickspacing for subfigures #20771

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
Merged
merged 1 commit into from
Aug 10, 2021
Merged

FIX: tickspacing for subfigures #20771

merged 1 commit into from
Aug 10, 2021

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jul 30, 2021

PR Summary

Closes #20765

tick spacing was incorrect for subfigure. The issue was pretty subtle, but the dpi transform was not correct in x/yaxis.get_tick_spacing when there were subfigures.

The new test fails on master with, and you can see that 300 dpi has different list of ticks than 120 dpi

E       AssertionError: 
E       Not equal to tolerance rtol=1e-07, atol=0
E       
E       (shapes (5,), (7,) mismatch)
E        x: array([-200., -100.,    0.,  100.,  200.])
E        y: array([-150., -100.,  -50.,    0.,   50.,  100.,  150.])

lib/matplotlib/tests/test_figure.py:1058: AssertionError

The test is a bit convoluted, but that is because it is tricky to get the ticks to fall right on the boundary where the difference can be seen.

@jklymak jklymak force-pushed the fix-tickspacing-sublayout branch from ebe6d09 to 357cfe5 Compare July 30, 2021 14:23
@jklymak jklymak added this to the v3.4.3 milestone Jul 30, 2021
@jklymak
Copy link
Member Author

jklymak commented Jul 30, 2021

Again, prob not urgent for 3.4.3...

@anntzer
Copy link
Contributor

anntzer commented Jul 30, 2021

Should we check for all instances of .figure.dpi in the codebase and test whether they need a similar fix?

@jklymak jklymak marked this pull request as draft July 30, 2021 17:14
@jklymak
Copy link
Member Author

jklymak commented Jul 30, 2021

Lets move to draft and I'll do a census as @anntzer suggests.

As a note, I notice that the new test also makes the markersize change with dpi, which I don't think happens with normal figures.

@jklymak
Copy link
Member Author

jklymak commented Jul 30, 2021

A quick scan doesn't show any other cases. Note figure.dpi appears a lot, but this was an axes-coords->pixels->points conversion. Most dpi conversions are pixels->points or pixels->inches, which don't care about the axes co-ordinates.

@jklymak jklymak marked this pull request as ready for review July 30, 2021 18:03
QuLogic
QuLogic approved these changes Jul 30, 2021
View reviewed changes
@jklymak
Copy link
Member Author

jklymak commented Aug 2, 2021

See #20777 for another case.

@jklymak jklymak marked this pull request as draft August 5, 2021 19:23
@jklymak jklymak force-pushed the fix-tickspacing-sublayout branch from 78956c3 to 3e7dc9b Compare August 5, 2021 22:58
@jklymak jklymak mentioned this pull request Aug 5, 2021
Closed
7 tasks
@jklymak
Copy link
Member Author

jklymak commented Aug 5, 2021

This now includes a setter/getter property for SubFigure.dpi the redirects to the parent. Other properties on the SubFigure that are inherited from the parent are already pointers....

@jklymak jklymak marked this pull request as ready for review August 5, 2021 23:48
@jklymak jklymak force-pushed the fix-tickspacing-sublayout branch from 3e7dc9b to 8a3ce1d Compare August 6, 2021 13:34
tacaswell
tacaswell reviewed Aug 10, 2021
View reviewed changes
lib/matplotlib/figure.py
def dpi(self):
return self._parent.dpi

@dpi.setter
Copy link
Member

Choose a reason for hiding this comment

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

not sure we want the setter here? The getter makes sense as we may need this in the transforms, but do we want to be able to change the dpi of the whole figure from a sub figure?

On the other hard it seems realtively harmless and makes sure that subfigures feel as like figures as they can.

Copy link
Member

Choose a reason for hiding this comment

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

I actually tried it, and it breaks some tests without the setter. I did not investigate any further though, so I don't know if that's a real problem or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to mess around with that. I'd say dpi is one of our more fragile concepts.

@tacaswell tacaswell merged commit b61e76a into matplotlib:master Aug 10, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 10, 2021
@tacaswell @meeseeksmachine
Backport PR matplotlib#20771: FIX: tickspacing for subfigures
5bf04bd
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 10, 2021
Merged
tacaswell added a commit that referenced this pull request Aug 10, 2021
@tacaswell
Merge pull request #20820 from meeseeksmachine/auto-backport-of-pr-20…
7d5754e 
…771-on-v3.4.x

Backport PR #20771 on branch v3.4.x (FIX: tickspacing for subfigures)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
topic: figures and subfigures topic: ticks axis labels
Projects
None yet
Milestone
v3.4.3
Development

Successfully merging this pull request may close these issues.

savefig re-scales xticks and labels of some (but not all) subplots
4 participants
@jklymak @anntzer @tacaswell @QuLogic