Skip to content

Don't change Figure DPI if value unchanged #16971

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
Closed
wants to merge 1 commit into from
Closed

Don't change Figure DPI if value unchanged #16971

wants to merge 1 commit into from

Conversation

dopplershift
Copy link
Contributor

So while digging into #15131, I noticed a change in how much we call resize. Here is the sample code I'm running:

import matplotlib
matplotlib.use('macosx')
import matplotlib.pyplot as plt
fig1, ax = plt.subplots()
plt.show()

When I run that 3.2.1, I see the following set of print outs of calls of various resize-related methods:

_macosx.View.windowDidResize
FigureCanvasMac.resize: 640 480 1.0
Figure.set_size_inches: False [6.4 4.8] 100.0
Figure._set_dpi 200.0 True
Figure.set_size_inches: True [6.4 4.8] 200.0
FigureManagerBase.resize

That looks reasonable. This is what the same code does on current master:

_macosx.View.windowDidResize
FigureCanvasMac.resize: 640 480 1.0
Figure.set_size_inches: False [6.4 4.8] 100.0
Figure._set_dpi 200.0 True
Figure.set_size_inches: True [6.4 4.8] 200.0
FigureManagerBase.resize
Figure._set_dpi 200.0 True
Figure.set_size_inches: True [6.4 4.8] 200.0
FigureManagerBase.resize
Figure._set_dpi 200.0 True
Figure.set_size_inches: True [6.4 4.8] 200.0
FigureManagerBase.resize
...[repeat last 3 lines 50 more times]

It looks like this change came in #16828. Prevously when Text.get_window_extent got a DPI of None, the function did not set figure.dpi. Now:

matplotlib/lib/matplotlib/text.py

Lines 894 to 913 in e702edd

if dpi is None:
dpi = self.figure.dpi
if self.get_text() == '':
with cbook._setattr_cm(self.figure, dpi=dpi):
tx, ty = self._get_xy_display()
return Bbox.from_bounds(tx, ty, 0, 0)
if renderer is not None:
self._renderer = renderer
if self._renderer is None:
self._renderer = self.figure._cachedRenderer
if self._renderer is None:
raise RuntimeError('Cannot get window extent w/o renderer')
with cbook._setattr_cm(self.figure, dpi=dpi):
bbox, info, descent = self._get_layout(self._renderer)
x, y = self.get_unitless_position()
x, y = self.get_transform().transform((x, y))
bbox = bbox.translated(x, y)
return bbox

If DPI comes in as None, the current DPI is grabbed and then reset on Figure. The change I've implemented here is to make setting Figure.dpi a no-op if the value isn't different. This gets us back to:

_macosx.View.windowDidResize
FigureCanvasMac.resize: 640 480 1.0
Figure.set_size_inches: False [6.4 4.8] 100.0
Figure._set_dpi 200.0 True
Figure.set_size_inches: True [6.4 4.8] 200.0
FigureManagerBase.resize

@dopplershift
Don't change Figure DPI if value unchanged
b7e881f 
Check that the value of dpi is different before going through all the
machinery to actually flush the change of value.
@QuLogic
Copy link
Member

QuLogic commented Mar 31, 2020 •
edited
Loading

Oh, this is the same conclusion I ended up at #16972, though slightly different implementation.

I don't mind rebasing on this one, but perhaps include a similar comment (or stuff from commit message).

@tacaswell
Copy link
Member

As pure style, I have a slight preference for the early return (less indentation, less code churn), but 🤷‍♀️.

tacaswell
tacaswell approved these changes Mar 31, 2020
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Either this or the implementation in #16972

@dopplershift
Copy link
Contributor Author

If #16972 needs to go in anyway for the gtk3 fixes, we can just merge that.

@tacaswell tacaswell added this to the v3.3.0 milestone Mar 31, 2020
@dopplershift dopplershift mentioned this pull request Apr 1, 2020
Merged
6 tasks
@anntzer
Copy link
Contributor

anntzer commented Apr 4, 2020

Superseded by #16972.

@anntzer anntzer closed this Apr 4, 2020
@dopplershift dopplershift deleted the figure-dpi branch April 12, 2020 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
status: duplicate
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dopplershift @QuLogic @tacaswell @anntzer