| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I think there needs to be a better way of ensuring the correct behaviour. If there is a .dpi attribute and a .set_dpi method you would expect that they are kept in sync. If that can't be achieved, the usual way would be to make the attribute private but keep the method public. I find it btw. a bit strange to adapt 40 year old nomenclature just because suddenly companies sell "high-res" gadgets. In that sense set_dpi should set the dpi. |
Sorry, something went wrong.
|
@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 fig.set_dpi; however, if a user sets the fig.dpi directly, they may be open to confusion. 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? |
Sorry, something went wrong.
|
Privatizing all the calls to figure.dpi to figure._dpi is daunting. |
Sorry, something went wrong.
|
Also ping @efiring who I think worked on a lot of this...? |
Sorry, something went wrong.
|
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 set_viewport_dpi(), but I haven't understood this in completeness. |
Sorry, something went wrong.
|
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 viewport_dpi not the nominal_dpi that the user specifies. So I'm proposing _dpi be the viewport_dpi (since that is the DPI wanted 99.9% of the time in the codebase) and calling the nominal_dpi nominal_dpi. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| 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.
This is not quite true, we also need to save the original to drop back to the correct dpi for savefig with dpi='figure'.
Sorry, something went wrong.
| self.patch.set_facecolor(color) | ||
|
|
||
| def set_dpi(self, val): | ||
| def set_dpi(self, dpi): |
There was a problem hiding this comment.
Please don't change argument names, it will break people doing **kwargs into it.
Sorry, something went wrong.
|
|
||
| # 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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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....
Sorry, something went wrong.
There was a problem hiding this comment.
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....
Sorry, something went wrong.
|
My initial thoughts on how to solve this were to make sure the public facing API (fig.dpi = N and fig.set_dpi(N)) both set _original_dpi and then give some way to by-pass setting _original_dpi when the dpi (as seen by the transfrom) is set by the GUIs. I think making a distinction between fig.dpi and fig.set_dpi is a bad idea. |
Sorry, something went wrong.
Most of the code base calls fig.dpi = N, though a few places called fig.set_dpi(N). As it stands now, that sets the actual dpi of the figure via the private function (dpi is a decorator) which is stored in fig._dpi. matplotlib/lib/matplotlib/figure.py Line 460 in c20b0c2 The rest of the code base calls dpi = fig.dpi, which just returns fig._dpi. So, we need to be able to set and get fig._dpi and it has to be the actual dpi, not the nominal one. 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 (fig._dpi) and the nominal one the user set (fig._original_dpi). And I really don't see how to make figure.set_dpi cater to both cases... |
Sorry, something went wrong.
|
@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 fig.set_dpi(100) and fig.dpi=100 do the same thing and still allow us to double the dpi internally for hi-dpi displays. The only way I can see to do it is to make a private property that controls the real dpi and have all the internal methods set/get that property. But maybe you have a good idea how to make it work. I'd actually be fine with removing (deprecating) figure.set_dpi and just make the user instantiate at figure-creation. I don't think there is any case where it makes sense to allow this to change on the fly, except perhaps when working interactively, but.... (note this doesn't mean you can't change the dpi for saving.) |
Sorry, something went wrong.
|
Closing to reduce cruft. The error in set_dpi remains, but not a high priority on my end. Anyone can feel free to resurrect this and/or make it their own... |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
As described in #11227, Qt5Agg interacts in strange ways with figure.set_dpi and the use of figure.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, it
is 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