Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH: Support units when specifying the figsize#29612
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b01a894 to62f489dComparelib/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd just accept both:if 'in' in unit:
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remark
If there could be a test for an invalidunit, I think this should be good to go!✔️
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 fine, but consider changing the test to the public API instead of private - someone could erase the call to_parse_figsize and we would never know the public API was broken?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
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.
Looks good!
Is this waiting on something else? |
Reviving the spirit ofmatplotlib#12402 andmatplotlib#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... |
4fa258a intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Alsocloses#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.