ENH: Support units when specifying the figsize #29612
Conversation
b01a894 to
62f489d
Compare
62f489d to
fea7c6f
Compare
lib/matplotlib/figure.py
Outdated
| if unit == 'inch': | ||
| pass | ||
| elif unit == 'cm': | ||
| x /= 2.54 | ||
| y /= 2.54 | ||
| elif unit == 'px': | ||
| x /= dpi | ||
| y /= dpi |
There was a problem hiding this comment.
Question
Centimetres and pixels are specified using the abbreviations 'cm' and 'px'. Inches are specified using 'inch'. For consistency, might it be more meaningful to use 'in' instead?
There was a problem hiding this comment.
Being a metric system aficionado, I don't have a strong opinion on 'inch' vs 'in'. I find 'in' a bit confusing because it is also a word in other context. 'cm' and 'px' are IMHO more clear. In the end, it likely doesn't matter too much, because it is the default anyway, so it's unlikely users will write this in their code explicitly. In the end this should be decided by people with an imperial background.
There was a problem hiding this comment.
I'd just accept both: if 'in' in unit:
There was a problem hiding this comment.
I'm a bit torn on what to accept. If we accept "in" and "inch", we probably should also accept "px" and "pixel", but then again, accepting "centimeter" feels a bit silly as that's very long and I don't expect anybody would write that, but for consistency, maybe we should? OTOH, the ValueError message is nice and tight now, adding all the variants would make it unwieldy
The unit part of 'figsize' must be one of 'inch', 'cm', 'px', but found 'foo'
vs
The unit part of 'figsize' must be one of 'inch', 'in', 'centimenter', 'cm', 'pixel', 'px', but found 'foo'
I'm stuck. Maybe then rather drop "inch" in favor of "in"?
There was a problem hiding this comment.
I don't think it hurts. However, one could take it to extremes of also including the plurals. So maybe let's just leave the simplest ("in") as you originally suggested and expand later if there is overwhelming demand.
There was a problem hiding this comment.
Ok, overcoming my awkward feelings towards imperial units, let's keep it simple and just use "in". After all, users won't type this often because it's the default.
lib/matplotlib/tests/test_figure.py
Outdated
| @pytest.mark.parametrize("input, output", [ | ||
| ((6, 4), (6, 4)), | ||
| ((6, 4, "in"), (6, 4)), | ||
| ((5.08, 2.54, "cm"), (2, 1)), | ||
| ((600, 400, "px"), (6, 4)), | ||
| ]) | ||
| def test__parse_figsize(input, output): | ||
| assert _parse_figsize(input, dpi=100) == output |
There was a problem hiding this comment.
Remark
If there could be a test for an invalid unit, I think this should be good to go!✔️
lib/matplotlib/tests/test_figure.py
Outdated
| ((5.08, 2.54, "cm"), (2, 1)), | ||
| ((600, 400, "px"), (6, 4)), | ||
| ]) | ||
| def test__parse_figsize(input, output): |
There was a problem hiding this comment.
I'm not a stickler for this, but don't we usually avoid unit tests for private methods, preferring to test the user facing functionality? This could be easily done by making a figure and then checking its size in inches, which is basically what you are doing here anyway.
|
Is this waiting on something else? |
Reviving the spirit of matplotlib#12402 and matplotlib#12415, because both had significant user votes on GitHub. This PR is intentionally minimal to only expand the `figsize` parameter when creating a figure. This should be the most relevant use case. Later changing the figure size or reading it is probably less necessary. The minimal approach removes the need to track and return sizes. It is just an enhanced specification capability which directly parses to the internally used inch unit.
|
This just needs a second review from a core developer. Unfortunately, review time is currently scarce. |
|
Feel free to self-merge if you detect the CI has finished before I do it... |
Reviving the spirit of #12402 and #12415, because both had significant user votes on GitHub. - #12402 is the highest up-voted issue that we have closed as "not planned" (17 up-votes); #12415 got 7 downvotes on the closing note that people should convert themselves.
Also closes #1369.
This PR is intentionally minimal to only expand the
figsizeparameter when creating a figure. It allowsplt.figure(..., figsize=(600, 400, 'px'))in addition to the currentplt.figure(..., figsize=(6, 4))- note that this passes through to all figure-creating methods. This should be the most relevant use case. Later changing the figure size or reading it is probably less necessary. The minimal approach removes the need to track and return sizes. It is just an enhanced specification capability which directly parses to the internally used inch unit.API considerations:
(width, height, unit)is a straight forward extension of(width, height). Discarded alternatives:figsize=((600, 400), "px")does not make it more readable and is very cumbersome to type.figsize="600x400 px"are difficult to parse and remember.figsize_unitparameter would be too verboseplt.figure(..., figsize=(6, 4), figsize_unit="cm")figsize_cm=...,figsize_px=...would lead to parameter explosion and are not very natural to useWe don't render us in a corner or have consisteny issues with other figsize-related API. There is only
Figure.set_size_inches()/Figure.get_size_inches()which are very explicit on the unit (maybe even more than reasonable, but that's a different discussion). If desired, we can always later addFigure.get/set_size()orFigure.get/set_figsize()that take a unit parameter.