Skip to content

NF - Added set_size_cm and get_size_cm for figures #5104

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

Closed
wants to merge 5 commits into from
Closed

NF - Added set_size_cm and get_size_cm for figures #5104

wants to merge 5 commits into from

Conversation

sameer
Copy link

@sameer sameer commented Sep 19, 2015

Provides for a "native approach" to figure sizing in centimeters, as requested in question 14708695 on StackOverflow
https://stackoverflow.com/questions/14708695/specify-figure-size-in-centimeter-in-matplotlib

@tacaswell tacaswell added this to the proposed next point release (2.1) milestone Sep 19, 2015
@tacaswell
Copy link
Member

Thank you for your contribution.

I am -.5 on this. It has come up before and was rejected for reasons I don't recall exactly.

If we do merge this, it should be a minimal pass-through to set_size_inches instead of a copy-paste.

@sameer
Copy link
Author

sameer commented Sep 19, 2015

I just made a change: instead of creating separate methods for metrics, I added an optional kwarg "metric" which is by default false for the get/set size functions. @tacaswell is this better?

@efiring
Copy link
Member

efiring commented Sep 19, 2015

While I appreciate the contribution as well as the motivation for handling metric units, I don't think that either form (two more methods, or a kwarg) is a good idea. One basic problem is that we already have a somewhat confusing situation with respect to units, with dots, points, relative font sizes, and inches. Adding in one more--cm--in a couple of places doesn't add much real functionality or usability, and doesn't make the situation more consistent or less confusing, but it does add a little more complexity. In addition, having a metric kwarg for a function with 'inches' in its name is not appealing.

@sameer
Copy link
Author

sameer commented Sep 19, 2015

@efiring I agree, it doesn't make sense to add a metrics argument to a set_size_inches function.
However, @tacaswell is against copying the methods, which is understandable. The additional kwarg does not affect current users of matplotlib, since it is only an addition.
Metric measurements are important -- matplotlib was built with US standards in mind. This addition makes life easier for metrics users.

@efiring , is there some alternative you would suggest?

@tacaswell
Copy link
Member

Having a metric kwarg is far worse. I was thinking more of

def set_size_cm(self, w, h=None, **kwargs):
    return self.set_size_inches(w*in_to_cm, h*in_to_cm if h is not None else None, **kwargs)

@sameer
Copy link
Author

sameer commented Sep 19, 2015

@tacaswell Looks good, I'll do that quick

@efiring
Copy link
Member

efiring commented Sep 19, 2015

On 2015/09/19 1:12 PM, Thomas A Caswell wrote:

def set_size_cm(self,w,h=None, **kwargs):
return self.set_size_inches(w_in_to_cm, h_in_to_cmif his not None else None,**kwargs)

What's the point of letting h be optional? This won't handle the
set_size_inches((w,h)) case. It's fine to leave that case out.

But I still think adding these methods is ad-hoc API bloat, and not a
good strategy. Example alternative: set_size(w, h, units="inches")
supporting inches, cm, points, pixels, and with deprecation of
set_size_inches. That's off the cuff, though; what's needed is a
careful look at how to handle units more consistently and flexibly
throughout mpl. In the meantime, requiring users to do their own
conversions between cm and inches is a minor inconvenience.

@sameer
Copy link
Author

sameer commented Sep 20, 2015

@efiring Shouldn't we be avoiding changing the name of the functions? There are already so many examples out there written with set_size_inches in the documentation and on StackOverflow.

@efiring
Copy link
Member

efiring commented Sep 20, 2015

Yes, something like set_size_inches will probably have to be around for a very long time. My point, though, is that if a function or method is going to be added, it would be best if it represents a genuine API improvement.

@mdboom
Copy link
Member

mdboom commented Sep 21, 2015

I wonder how @rmorshea's work on traitlets might help us here. If everywhere we accepted a physical length we could specify a unit, that would be universally consistent, localize the complexity and be the most flexible (likely). @rmorshea or @ellisonbg: I'd appreciate your thoughts about using traitlets for physical units.

@rmorshea
Copy link
Contributor

Probably the best way would be to have a TraitType which would accept numbers and strings. Strings passed to the attributes of that TraitType like '2.5cm' would be converted into the universal unit while ints and floats would be assumed to already be of that universal unit. If you're interested in retrieving that value in any particular unit, probably best to have a function get_size(unit) which defaults to the universal unit.

@efiring
Copy link
Member

efiring commented Sep 22, 2015

@rmorshea I prefer to avoid passing numbers as strings; perhaps an alternative would be to use a tuple in which the first element would be the numeric value and the second would be the unit.

@rmorshea
Copy link
Contributor

@efiring tuples seem reasonable.

An entirely different option would be to have a set_unit(name, unit) function which would set the units for input/output values of an attribute (underneath the hood this would be similar to how trait notifiers are handled). The underlying value would be of the universal unit, but that would be hidden from the user by getter and setter esk logic. The benefit being that there wouldn't need to be a get_size function (which in the other option need to be repeated for every attribute).

@paalge
Copy link
Contributor

paalge commented Oct 3, 2016

What is the status of this pull request?
I feel that since matplotlib is used all around the world it is strange that it by default chooses to use a unit not part of the SI units. This has been discussed before (https://sourceforge.net/p/matplotlib/mailman/message/9894585/), where it was pointed out that in some part of the printing business inches are the norm, but that does not mean that user needs to use it. As suggested there matplotlib could handle this internally if some internal methods require inches.

@tacaswell
Copy link
Member

Internally, the size of a figure is stored in inches (and will probably stay that way until 'dpc' overtakes 'dpi' in the rest of the world and 'point' is not defined as 1/72 of an inch).

The question is if the extra API size is worth users who prefer to work in cm to not have to do a conversion them selves.

It looks like this needs a bit more input cleanup in set_size_cm to handle the many allowable inputs.

I am still -.5 on this. I would not merge it, but would not protest if someone else did.

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Sep 24, 2017
@tacaswell
Copy link
Member

Coming back to this a year later, I am mostly convinced by @efiring 's suggestion of adding set_size(w, h, *, units='inches') and get_size(units='inches').

@jklymak
Copy link
Member

jklymak commented Sep 25, 2017

Personally, I'd be interested in a more general parsing of units for most things. I wrote a little units package that converted strings like xx.xin, yy.ycm, zz.zpt to appropriate floats. I think that would be welcome over a broad range of matplotlib methods. Internally, of course, the classes should store in what makes sense.

@tacaswell
Copy link
Member

@dopplershift also maintains (at least one) unit library

@dopplershift
Copy link
Contributor

It'd be more accurate to say I'm a very invested user in the pint unit library. I don't think it's useful to rely on a full math capable, dimension tracking library when all we need internally is handling units for length.

I don't have much of an opinion on set_size(w, h, *, units='inches') vs. @jklymak 's suggestion of a small unit package, which would essentially abstract things further.

@jklymak
Copy link
Member

jklymak commented Sep 25, 2017

@dopplershift The only reason for a small unit package is that there are many many other places where a dimension is asked for, and in many of them a unit could be useful. i.e. all the various paddings are specified in different units. It would be nice to allow all those dimensions to be specified in a library-consistent way.

I also think it'd make user code more readable. do_some_layout(h_pad=0.04) is not as helpful as do_some_layout(h_pad="3.0pts"), do_some_layout(h_pad="0.05rel") etc...

I suppose you could do this with units= in each call, but a) that doesn't let you mix units in a call which may be desirable in some circumstances (i.e. you want one unit to be in fixed units, and another to be in figure-relative). b) the default won't be explicit, and would still be subject to each module writer's whim.

@jklymak
Copy link
Member

jklymak commented Sep 25, 2017

Added a MEP: #9226

@jklymak
Copy link
Member

jklymak commented Oct 6, 2018

Closing in favour of set_size PR... #12415 Thanks a lot for the contribution and discussion, I think this is a good idea...

@jklymak jklymak closed this Oct 6, 2018
@story645 story645 removed this from the future releases milestone Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants