Skip to content

Prefer add_subplot(foo=bar) to subplots(subplot_kw={"foo": bar}). #14411

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 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jun 1, 2019

When creating a single subplot I think the former is more readable.

(Note that before mpl3.1 one had to write add_subplot(111, foo=bar)
where the tradeoff was less clear.)

Happy to discuss which form to prefer...

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Copy link
Member

@jkseppan jkseppan left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@timhoffm
Copy link
Member

timhoffm commented Jun 2, 2019

+0.5. I think, this is more readable for the cases presented here.

I'm a bit worried on the ax = plt.figure().add_subplot(projection='3d') chaining. This is not a pattern commonly used in matplotlib so far. And explicitly creating a figure without binding it to a variable may appear a bit pedantic. In the pyplot world (which you are sort of in here even if you are later only working on the axes, this could simply be ax = plt.subplot(projection='3d').

So if we're going away from "just use plt.subplots() and then work with the axes", I think we can equally allow plt.figure() and plt.subplot().

@efiring
Copy link
Member

efiring commented Jun 3, 2019

The advantage of plt.figure().add_subplot() over plt.subplot() is that it makes the figure creation explicit, and the chain is easily split if it turns out that one needs a reference to the figure. It takes more typing, but I think that it might in the end help new users understand mpl more quickly than if they rely on the additional pyplot magic in plt.subplot().

@timhoffm
Copy link
Member

timhoffm commented Jun 3, 2019

I agree that the explicit figure creation has its merit, and partly withdraw my argument that plt.subplot() and plt.subplots() should be used equally (it's unfortunate that these functions are named similarly, but have a different behavior with respect to figure creation).

This whole axes creation is a bit of a mess. If we did not have history

ax = plt.figure().add_subplot()
axs = plt.figure().add_subplots()

would look like a reaonable API to me (maybe with optional pyplot shortcuts using implicit figure creation). But we're not there. The latter is plt.figure().subplots() and we've advertized plt.subplots() instead.

I'm really worried we're confusing users with all these different methods and changed recommendations for axes creation. Even if plt.subplots(subplot_kw={"foo": bar}) is ugly, at least it's the straight forward extension of plt.subplots() / plt.subplots([2, 2]). Making the former look nicer but in exchange break API similarity with related functions is not a big win (if any).

Essentially, I see two reasonable ways:

  1. Keep as is for now to not have yet another partial change; i.e. stay with plt.subplots() for now.
  2. Rethink the whole Axes generation and establish one consistent API. This could be:
  • Create Figure.add_subplots() as a synonym for Figure.subplots() (deprecate or keep the latter).
  • establish
    ax = plt.figure().add_subplot()
    axs = plt.figure().add_subplots()
    
    as a canonical way to create axes. Split the call if you need a reference to the figure. Optionally use pyplot functions as a shortcut. Should that be plt.subplot[s]() or plt.add_subplot[s]()?

This is just an of-the-top of my head concept and certainly would need more thorough consideration.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 3, 2019

Well, I would really have preferred to have Figure.add_subplots instead of Figure.subplots but I lost the discussion at #5139 #5146 :(

I agree that ever-changing recommendations are not great :( but OTOH we're all (well, at least I'm definitely still) learning how to best design an API.

I guess this discussion is also colliding with #14421 now.

@tacaswell tacaswell added this to the v3.2.0 milestone Jun 8, 2019
@jklymak
Copy link
Member

jklymak commented Jul 5, 2019

I'm not a fan of favouring a different API for a single subplot than for multiple subplots, and I'm not at all a fan of add_subplot which is really just a matlab-ism. OTOH, I agree that subplot_kw is pretty ugly, and makes those kwargs non-discoverable. Would be good to talk about these use cases in one place and come up w/ what we think should be canonical?

@timhoffm timhoffm modified the milestones: v3.2.0, v3.3.0 Aug 15, 2019
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
@jklymak jklymak marked this pull request as draft August 24, 2020 14:24
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
@tacaswell tacaswell modified the milestones: v3.5.0, v3.6.0 Aug 5, 2021
@timhoffm timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@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 Jun 19, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Jun 19, 2023

@efiring @timhoffm Do we want to use this pattern at least in some cases? If not, let's just close the PR.

@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label Jun 21, 2023
@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 21, 2023
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I'm still very undecided between subplots(..., subplot_kw=...) and figure().add_subplot().
Personally, I likely wouldn't actively rewrite this, but also wouldn't block if you push this forward again.

We should definitively limit the kwargs to polar and 3d. Everything else (see suggestions) should not be crammed into the subplots creation. This is a change that can be made independently of above decision.

@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label Aug 23, 2023
When creating a single subplot I think the former is more readable.

(Note that before mpl3.1 one had to write `add_subplot(111, foo=bar)`
where the tradeoff was less clear.)
@anntzer
Copy link
Contributor Author

anntzer commented Aug 23, 2023

OK, I rewrote the whole thing to just make the changes that should be uncontroversial.

@story645
Copy link
Member

Uh I hate to jump in this late but I'm concerned about the ax = plt.figure(). construction hiding the figure.

is that it makes the figure creation explicit, and the chain is easily split if it turns out that one needs a reference to the figure

My concern is w/ the folks who won't realize it's a chained call...I know the () should be a giveaway but I don't think it will be. I'd rather go all the way explicit here and use the unchained construction and leave "you can chain" to a section of the user guide or a tutorial.

@story645 story645 added the Documentation: examples files in galleries/examples label Aug 23, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Aug 23, 2023

I think a crucial point of python semantics as opposed to matlab (which is still pretty commonly used in my field) is exactly that you can chain calls, i.e. tmp = foo(); b = tmp.bar() is equivalent to b = foo().bar() and therefore you explicitly don't need to litter your code with all the intermediate temporaries (and another crucial point is that parentheses mark function calls, which again isn't the case in matlab (which implicitly calls functions with no args)).
I would say the chained call is proper style; if there's opposition to it I'd rather just close the whole thing rather than writing fig = plt.figure(); ax = fig.add_subplot(...) which is worse IMO.

@story645
Copy link
Member

I would say the chained call is proper style;

I don't disagree in the case where they're definitely not going to use the figure object, but my concern is that this is example gallery code where the expectation is folks will c&p and then tweak.

I think I agree w/ @jklymak that we should probably pin down a standard.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 24, 2023

Let's just close this until we can actually decide on a standard.

@anntzer anntzer closed this Aug 24, 2023
@anntzer anntzer deleted the subplot_kw branch August 24, 2023 07:10
@esibinga
Copy link
Collaborator

re: deciding on a standard:

Is there an existing standard for constructing a figure / axes / subplots? I see this in Getting Started:

fig, ax = plt.subplots()
ax.plot(x, y)
plt.show()

Given that figure/ax construction is both super confusing for beginners and ubiquitous for using mpl, I think that specifically warrants a standard that the gallery examples really stick to. That way users can focus on the feature they're looking for, without having to detangle how the figure is constructed. If a different setup is unavoidable or nonsensical for 3D projections etc, it may be worth anticipating confusion by adding a link at the bottom of the example page to the quick start guide or another tutorial that explains the difference in setup.

That seems to me to be potentially a separate question from a standard about using chained calls in general. I'm ambivalent there -- it seems easier to figure out when it's later on in the code, but I would avoid shortcuts in the setup.

And FWIW, personally I think fig and ax should always be explicit in the gallery examples because that setup is already confusing enough if you're not a frequent user. It is a little jarring to only see ax in this example after looking at several others that use both fig and ax.

@jklymak
Copy link
Member

jklymak commented Aug 24, 2023

We have already added width_ratio and height_ratio as pass throughs. The only one I really miss is facecolor

I think in general passing the figure back to the user is quite often necessary even in the case of a single subplot - in particular for colorbars. We could have ax.colorbar but again, having a separate idiom for single axes versus multiple axes in the docs seems confusing. I think we should always use fig, axs = plt.subplots() for consistency, even if there is just one axes.

timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Aug 30, 2023
While technical possible, this is an anti-pattern IMHO.
We should keep Axes creation and customization separate.

Extracted from matplotlib#14411.
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Aug 30, 2023
While technical possible, this is an anti-pattern IMHO.
We should keep Axes creation and customization separate.

Extracted from matplotlib#14411.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: examples files in galleries/examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants