-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
FIX: fix figure.set_dpi when pixel ratio not 1 #11232
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
Conversation
211f84f
to
1bd9a79
Compare
I actually wonder if we should reduce confusion by deprecating figure.set_dpi and call it figure.set_nominal_dpi. That would stop developers from using it accidentally and not have a confusing overlap with the fig.dpi property. |
I think there needs to be a better way of ensuring the correct behaviour. If there is a I find it btw. a bit strange to adapt 40 year old nomenclature just because suddenly companies sell "high-res" gadgets. In that sense |
@ImportanceOfBeingErnest we need two dpis: a nominal one and the actual one because many folks use two screens, one w/ hi-dpi and the other w/o; the clever thing about what has been done in the qt5Agg backend is it detects at draw time which is being used and adjusts the real dpi on the fly based on the nominal and whether the screen is double-resolution. I'm all for deprecating Currently (with this PR)fig._original_dpi # nominal dpi set at creation time, or when the user sets the dpi using set_dpi
fig._dpi # the *actual* dpi of the figure
fig.dpi # decorator with the *actual* dpi of the figure setter and getter (fig._set_dpi, dig._get_dpi).
fig.set_dpi # sets the *nominal* dpi and resets the *actual* dpi (fig._dpi) Option:fig.dpi # nominal dpi with set/get methods.
fig._dpi # actual dpi for internal use only. I guess that the second option is better and keep all the pixel doubling private? |
Privatizing all the calls to |
Also ping @efiring who I think worked on a lot of this...? |
So if what you call "option" is actually possible, that would be exactly what one would expect to have. I mean dpi is a figure property. It's not like you would change that property when moving a window that displays a figure to another screen. What you would change may be called a "viewport_dpi" or similar. If that viewport dpi is something a user needs to know about, one could also introduce |
I think "Option" is possible. I just did it. But whether I introduced a bunch of bugs in doing so is the question.... I appreciate and share your frustration. OTOH, the way the whole codebase is organized, we need to know the |
Quick reaction: even before the advent of hi-dpi/retina screens, I have been frustrated by the different meanings and uses of dpi in matplotlib, most evident in the difference between vector and bit-mapped backends, and screen versus print display. The direction we need to move is towards different names for the various types of "dpi", both internally and externally. Internal changes (private API) are easier, of course. The first step is to clearly identify and define the various "dpi-like" parameters. |
I think right now its this way:
This isn't conceptually difficult now that I suffered through it all. But it means we have an API problem, because w/ the hi-dpi we do not really want users specifying the dpi, we want them to specify a nominal dpi that then we change if we need to. |
specifies dimensions and dpi to matplotlib as if the resolution | ||
were not doubled. The default figure | ||
value is nominally 100 dpi, but displays that are deignated "hi-dpi" | ||
will internally double this to 200 dpi. We keep the "nominal" dpi |
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 is not quite true, we also need to save the original to drop back to the correct dpi for savefig with dpi='figure'
.
@@ -964,13 +965,44 @@ def set_facecolor(self, color): | |||
""" | |||
self.patch.set_facecolor(color) | |||
|
|||
def set_dpi(self, val): | |||
def set_dpi(self, dpi): |
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.
Please don't change argument names, it will break people doing **kwargs
into it.
|
||
# some backends set self._dpi to be a ratio times | ||
# self._original_dpi: | ||
ratio = self._dpi / self._original_dpi |
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.
I am not sure that we want to rely on this here. I can not produce one off the top of my head, but this feels like it is prone to race conditions.
This dpi doubling is the business of the GUI backends and I would like to keep as much of it there as possible. What we should do is set the _original_dpi
and then let the GUI know and have it sort the doubling out.
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.
Well race conditions can maybe be cured by clamping down the API a bit. See discussion above.
The figure
object needs to know about the dpi doubling because thats how all the drawing gets done.
We could temporarily set the dpi to twice at draw time.... Hmmmm. let me look into that....
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.
Oh, no its not that simple because of interactivity. I think it really is better to actually set the dpi for the figure object so it knows its actual rendered dpi....
My initial thoughts on how to solve this were to make sure the public facing API ( I think making a distinction between |
Most of the code base calls matplotlib/lib/matplotlib/figure.py Line 460 in c20b0c2
The rest of the code base calls So, we need to be able to set and get But we also "want" to let the user set the nominal dpi. Right now we can't do this via fig.set_dpi because that sets the actual dpi and causes all the inconsistencies. I don't see any way to make this all work without figure knowing about the difference between the real dpi ( |
@tacaswell I thought about this, and I don't see how to do what you are suggesting above. I can't figure a way to make I'd actually be fine with removing (deprecating) |
Closing to reduce cruft. The error in |
PR Summary
As described in #11227, Qt5Agg interacts in strange ways with
figure.set_dpi
and the use offigure.savefig(dpi='figure')
.This was due to the fact that if you have a Retina/hi-dpi screen, Qt5Agg was calling the private
figure._set_dpi(dpi, forward=False)
to a doubled dpi from the one specified by the user and/or the rcParam, and saving the "_original_dpi" (or nominal dpi) on the figure.Here the suggestion is that the figure should set its own nominal dpi at creation and that
figure.set_dpi
should only change the nominal dpi. If the dpi had been doubled by a backend, itis detected at this point and doubled internally by setting
fig.dpi = nominal * factor
.Fixes the QT5Agg part of #11227 ping @tacaswell @ImportanceOfBeingErnest
Before
After
PR Checklist