Skip to content
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

Add set_ & get_size_pixels #18611

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

takluyver
Copy link
Contributor

This is intended to forward a discussion, rather than as something ready to merge. So I've skipped most of the heavy lifting (tests, docs etc.) for now - I'm happy to do it if the feature is acceptable in principle.

Like other people (#12402, #12415, #9226, and doubtless others), I'm frustrated that matplotlib forces me to express things in inches in scenarios which are otherwise completely metric. I was about to make a PR with set_size_mm, basically the same idea as #12402.

Scanning those issues and PRs, I see the argument that inches (and the associated 'points') are commonly used in printing. However, the vast majority of plots I make are displayed on screen and never printed, so any physical measurement is a fiction anyway (even if I set dpi accurately for one monitor, moving the plot to my other monitor will make it wrong). The docstrings for get/set_size_inches tell me how to convert to & from pixels, but in practice I just fiddle with the numbers until it looks about right. I'd argue that the size in pixels is important enough to warrant convenience methods like this, rather than making the caller deal with dots-per-inch.

I see there has been discussion in the past of a more general units system, like CSS sizes, where you could specify '200 px' or '10 cm'. I agree with the principle of that, but I don't think I'd ever find the time to design, advocate & implement it.

@jklymak
Copy link
Member

jklymak commented Sep 30, 2020

The problem with this proposal is that DPI can, and often does change on output. With the default dpi=100 the following will save an 800x800 pixel figure:

    fig.set_size_pixels(400, 400)
    fig.savefig('boo.png', dpi=200)

whereas I think it promises a 400x400 figure.

@story645 story645 marked this pull request as draft September 30, 2020 19:08
@timhoffm
Copy link
Member

timhoffm commented Sep 30, 2020

@takluyver I see where you are coming from.

I'm known for being largely opposed to adding sizing in other units, because I have yet to see a concise, consistent and backwards-compatible way of doing it (c.f. #12402 (comment)).

Pixels would actually be the only one theoretically worth considering, because most backends use this as a native unit.

However, I still have concerns:

  • In generalization to @jklymak's valid remark: Matplotlib is internally saving the size in inches. Therefore, the size in pixels is not a conserved quantity under dpi changes:

    fig.set_size_pixels(400, 400)
    fig.dpi = 50
    fig.get_size_pixels()  # returns 200, 200
    

    which is surprising.

  • Is Figure.set_size_* really used in the wild? I assume that 99% of figure sizing happens on figure creation through the figsize parameter. In that case, this PR is little gain. OTOH, I don't see how we can reasonably support pixels on figure creation.

@takluyver
Copy link
Contributor Author

I assume that 99% of figure sizing happens on figure creation through the figsize parameter.

Yes, in fact that's how I usually do it as well. I focused on the method just because there's an obvious way to add a similar method with different units. But I understand that it has its problems. I would love to have a way to not deal with inches, but I guess this is a topic that you've all been through so many times that you're thoroughly sick of it by now!

@tacaswell
Copy link
Member

I find the pixel based version of this more compelling than the metric based versions (which admittedly maybe an artifact of my American imperial unit mindset, but I feel better that @timhoffm agrees) because, for raster outputs, is the fundamental unit of the output.

Even though I agree with @takluyver that a vast majority our render go straight to a screen or to a website, internally the our print heritage is deeply embedded in the internals (in that the Truth is the size in inches and the dpi, the pixel count is strictly derived). This also seems more consistent with our ability to go to vector outputs as well as raster (as we understand how to talk about the absolute size of a raster output, but less about the pixel count of a vector output, vector outputs tend to actually be in points (which is defined as 1/72in), and we don't know which version the user is going to want to save until they actually save). Which is a very long way of saying I don't think that changing our internal version of the size to pixels is a good idea.

Because we treat the size in inches and the DPI as "set by the user" and the pixel size as strictly derived we get out of the issue of having to decide which to change if the user changes the third or what we do if the user gives us impossible requests (e.g. 5 by 5 inches, 200 dpi, and 150 by 250 pixels). This PR makes the choice that "we always change the size in inches, not the DPI", but you could imagine defending the position that this should change the DPI instead.

That said, having an alias for setting the size in pixels seems reasonable. I am not too concerned with the fig.savefig(..., dpi=200) case that @jklymak brought up as we already have bbox_inches which lets you resize / pull out a sub-section of the figure. It is also extremely visible in the code, so so long at is it very clear that this is a helper, not the definitive value does not seem like too much a problem.

On thing that is worrying here is that we have the savefig.dpi rcparam that defaults to 'figure' (which means "use the dpi the figure thinks it has"), but if it is the non-default value you would silently (from reading the code) land us in the same problem.

In my personal work I use set_size_inches pretty regularly and it is what is called under the hood when you resize a GUI window.

@jklymak
Copy link
Member

jklymak commented Oct 1, 2020

Exposing the idea that pixels are a fundamental user-facing dimension leads to many misconceptions and confusion on the part of users. We should be striving for screen resolution independence, and leave the idea of pixels to whatever does the rendering, be it a GUI backend or a website. Of course thats not 100% practical for a raster backend, and we need to specify the dpi at some point, but it should be late in the game when rendering the figure, not when creating it.

@asdfCYBER
Copy link

asdfCYBER commented Apr 18, 2021

I have been ever so slightly annoyed by MPL forcing me to think in inches for the few years that I've used it, and I've come here to find whether a request for customizable units has already been made. That seems to be the case, but they all seem to have stranded somewhere. Has there been recent progress?

Here's my (possibly unfeasible) idea which I haven't seen elsewhere yet (also partially to kick this discussion back to life): add a generic get_size and set_size to Figure with optional unit = "inch" argument, and allow the default to be changed with styles/rcparams

@timhoffm
Copy link
Member

timhoffm commented Apr 18, 2021

More (lengthy) discussion in #12402 and #12415.

Here's my (possibly unfeasible) idea which I haven't seen elsewhere yet (also partially to kick this discussion back to life): add a generic get_size and set_size to Figure with optional unit = "inch" argument, and allow the default to be changed with styles/rcparams

I would be inclined to accept the minimal approach Figure.get_size(*, unit='inch') / Figure.set_size(w, h, *, unit='inch') with

  • no change to the figsize keyword argument (i.e. it's still always inches) - That's part of why ENH: fig.set_size to allow non-inches units #12415 stalled.
  • no rcParam for changing defaults - I have serious concern that such a parameter could create more chaos than it's worth. Also, the internal mechanism is inches and changing dpi changes the pixels. It's sort of ok to set/get pixels at the time of the function calls, but we can't pretend that pixel is an inherent property of the figure.

@tacaswell
Copy link
Member

Coming back to this, I agree with @timhoffm 's comment #18611 (comment) as the least-bad way to get both pixels and cm (although from quick googling, it looks like metric typography is based on mm) support added. It is always clear at the point of call what unit the user is providing / expecting. We should also make clear in the docstring that the size in inches + dpi is the "ground truth" of size / density of the figure.

@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: inactive Marked by the “Stale” Github Action status: needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants