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

Lim parameter naming #11293

Merged
merged 2 commits into from
Jun 18, 2018
Merged

Conversation

timhoffm
Copy link
Member

PR Summary

In #11137 the kwargs *xlim of set_*lim were deprecated. This PR removes their occurrences from the code base and examples.

I'm not quite happy with the deprecation, because xmin, xmax, ymin, ymax feel more straight forward and are easier to remember than left, right, bottom, top. Anyhow, if we want to go that way, the code itself should be consistent.

@ImportanceOfBeingErnest
Copy link
Member

Did anyone ever think this deprecation through?

What is left and right on a polar axes?

@timhoffm
Copy link
Member Author

The deprecation was briefly discussed in #11137 (comment) and the following two comments.

@jklymak
Copy link
Member

jklymak commented May 23, 2018

Its unreleased - we can always back it out if there is a strong reason to not deprecate these names.

@timhoffm
Copy link
Member Author

Some deprecation is reasonable. There shouldn't be two keywords with the same functionality. The question is what the good names are. Currently we support the following kwarg names.

limit min / max directional
xlim xmin, xmax left, right
ylim ymin, ymax bottom, top
zlim zmin, zmax bottom, top
rlim rmin, rmax
thetalim thetamin, thetamax

There's been a slight preference to keep the directional names in the discussion in #11137 (review). However, it was not discussed to a broad extent and there were only three opinions given.

@anntzer
Copy link
Contributor

anntzer commented May 23, 2018

On polar axes there's always rmin/rmax; thetamin/thetamax which didn't go away.

I think I'm -0 on restoring xmin/xmax for simplicity and for reasons given in the thread at #11137 (comment) (@timhoffm you said yourself that two ways of doing it was a bit silly :-)), but don't feel very strongly about it either, so let's collect some opinions...

@anntzer anntzer added this to the v3.0 milestone May 23, 2018
@jklymak
Copy link
Member

jklymak commented May 23, 2018

I'm against two kwargs meaning the same thing.

Just to be contrarian, I'd have used lower/upper as in "lower bound" and "upper bound" for all three axes....

@timhoffm
Copy link
Member Author

To state my position clearly (and add to the diversity):

  • I'm against two kwargs meaning the same thing.
  • I'd keep all the *min/max and deprecate left, right, bottom, top.

@jklymak
Copy link
Member

jklymak commented May 23, 2018

If I have to chose, I'd choose *min/max as well...

@ImportanceOfBeingErnest
Copy link
Member

Due to the arguments handled in a more or less positional way, i.e. the left argument denoting the left boundary, I do think that left, right, top, bottom is actually "more correct". This is also expressed by @anntzer's comment

one can pass left > right for inverted axes, which sounds less silly than xmin > xmax...

This means that ax.set_xlim(left=3, right=1) is actually correct, language wise as well as code wise, while ax.set_xlim(xmin=3, xmax=1) is very strange to say the least.

However what bites me here is with polar plots where there is no left or right. I at least happen to always use set_xlim and set_ylim on polar plots as well (because I can never remember the other methods' names to do the same thing). But maybe I shouldn't?

Doing this from scratch I would probably favour something like start and end, or so, such that the same keywords can be used with every such *lim function. Not sure if this is possible at this stage.

In any case whatever is decided I think it should go through a deprecation period of at least a year or so, to make sure people have time to adapt their codes.

@anntzer
Copy link
Contributor

anntzer commented May 24, 2018

As a side note, set_rlim and set_thetalim also need to be updated (in polar.py), as they remap the rmin/rmax/thetamin/thetamax kwargs to xmin/xmax/ymin/ymax, and thus trigger a deprecation warning.

@afvincent
Copy link
Contributor

FWIW, I kind of like @jklymak's suggestion about the keywords lower/upper. Would this not allow to have an identical naming scheme across all the limit setting methods (for cartesian coordinates, as well as other projections)? Personally, I find it a bit disturbing that one has to repeat the axis name in the keyword name (e.g. set_xlim(xmin=...) instead of set_xlim(min=...), although I guess that the latter may be prone to overload min and max built-ins).

@efiring efiring self-requested a review June 4, 2018 19:28
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is moving in the right direction; additional discussion with @tacaswell is recommended.

@timhoffm timhoffm added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 17, 2018
@timhoffm
Copy link
Member Author

timhoffm commented Jun 17, 2018

Marking as "Release Critical". We must not internally use deprecated APIs. Otherwise the users will see the deprecation warnings we're generating with that.

Either this PR has to go in for 3.0 or the deprecation of the *min, *max (part of #11137) has to be rolled back.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you.

@tacaswell tacaswell merged commit 5a927c1 into matplotlib:master Jun 18, 2018
@timhoffm timhoffm deleted the lim-parameter-naming branch June 18, 2018 20:40
@pierre-haessig
Copy link
Contributor

Hi,

It's only pretty recently that I discovered the deprecation of xmin, xmax arguments since I'm working with stable releases of Matlplotlib and I only "conda update -all" every month or so. I was pretty surprised, because over the years, I've been consistently using these keyword args and I had literally forgotten that the left and right arguments existed anyway!

So I got rather frustrated by the deprecation, and thus I dig in the PRs. What I read didn't convinced me:

  • original PR Convert **kwargs to named arguments for a clearer API #11137 *"Convert *kwargs to named arguments for a clearer API "

    • first, of course, the purpose of the PR was not to change any API.
    • xmin/max got depecrated far in the middle of the discussion, but only with a very low level of motivation: "Would it be worth deprecating ...?". The reason is purely aesthetical (once the genuine issue of the silent overwrite between the synonymous arguments is settled of course.).
    • then there is a debate whether left is clearer than xmin of vice versa: which one should get deprecated after all?
    • finally deprecation is done while said to be "tangential to the purpose".
    • → As an external reader, it seems that this deprecation was close to the outcome of a dice roll!
  • in the present PR, it is stated that the newly prefered notation left/right makes no sense in other contexts like polar plots

  • later, in Still naming inconsistency in API on axes limits #11761, @timhoffm uncovers that the xmin terminology is in fact used elsewhere.

So my conclusion is that the "consequence analysis" (sorry, I don't have the proper word for this) for this deprecation was a bit short and I'm not sure the aesthetical reason (the duplicate keywords which I agree is odd) is worth breaking piles of existing codes (assuming I'm not the only weird addict user of xmin, xmax, ymin, ymax ;-) ). Am I indeed lonely?

@jklymak
Copy link
Member

jklymak commented Jan 25, 2019

Yeah, I personally think it was a mistake. Can we carry on the conversation in the open issue #11761 though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants