Skip to content

DOC: remove examples of subplot(123) from docs #17335

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

DOC: remove examples of subplot(123) from docs #17335

wants to merge 7 commits into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented May 5, 2020

PR Summary

Changes all *subplot(123, to subplot(1, 2, 3 in the docs and code base.

Sorry, I dislike these thrashy PRs as well, but feel we should really soft-deprecate this usage...

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

anntzer
anntzer reviewed May 5, 2020
View reviewed changes
doc/api/prev_api_changes/api_changes_0.54.rst Outdated
@@ -92,7 +92,7 @@ Axes creation
use::

f = Figure(figsize=(5,4), dpi=100)
a = f.add_subplot(111)
a = f.add_subplot(1, 1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly think we should not change old API changes notes -- they should stay representative of what the lib was back when that release occured.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Well, I kept a couple of the really recent ones...

anntzer
anntzer reviewed May 5, 2020
View reviewed changes
doc/devel/documenting_mpl.rst Outdated
@@ -820,7 +820,7 @@ example is generated from
s = 1 + np.sin(2 * np.pi * t)

# Note that using plt.subplots below is equivalent to using
# fig = plt.figure and then ax = fig.add_subplot(111)
# fig = plt.figure and then ax = fig.add_subplot(1, 1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that add_subplot(111) can be spelt even more succintly as add_subplot() now (it defaults to 111). Especially when you're clearly not going to have a single subplot, I think the argless version is actually the cleanest.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed.

@timhoffm
Copy link
Member

timhoffm commented May 5, 2020

Semi-OT: IMHO subplot(m, n, p) is only marginally better than subplot(mnp). Figuring out what the p-th element always confuses me and is a lot of cognitive overhead. Unless we're making a point on using pyplot (which I think has subplot as the canonical way of adding multiple plots) we should switch to subplots().

@jklymak
Copy link
Member Author

jklymak commented May 5, 2020 •
edited
Loading

@timhoffm I agree, but thats a far bigger job.

And the plt.subplot(m, n, p) grammar is very ingrained, and is at least well-defined.

Conversely subplot(mnp) is a really old grammar that causes genuine problems because it only applies to 9 or fewer rows and columns subplots.

@timhoffm
Copy link
Member

timhoffm commented May 5, 2020

Anybody using mnp or m, n, p with p > 9 must be a genius or a masochist. 😆 But I agree that we should do the change proposed here.

BTW, we could add p = i, j so that subplot(4, 4, (0, 3)) would be the top right edge. Would need to decide on 0-based vs. 1-based here, which is a bit unclear as the scalar p is 1-based, but I still would use numpy/matrix indexing. But that's for another time.

@matthew-brett
Copy link
Contributor

matthew-brett commented May 6, 2020 via email

@anntzer
Copy link
Contributor

anntzer commented May 6, 2020

subplot(i, j, (k, l)) basically already exists, except that it's spelled subplot2grid((i, j), (k, l)) (zero-based indexing). I guess subplot2grid is so obscure though that also having the functionality in subplot may not be a bad idea? (not that I claim any understanding of what is good taste in a pyplot-centered workflow...).

@timhoffm
Copy link
Member

timhoffm commented May 6, 2020 •
edited
Loading

And when you think you know all the miraculous ways you can create an Axes, there‘s always one more API ... 😩. I‘m really not sure we should change any of these API anymore unless we have a greater plan which and how these should be used (and maybe soft-deprecate some).

This is only referring to m, n, (i, j) not to the contents of this PR.

@jklymak
Copy link
Member Author

jklymak commented May 6, 2020

Well, we already have m, n, (start, stop), at least until it was broken ;-) Please see #17343

@efiring
Copy link
Member

efiring commented May 7, 2020

I would be happy to see the 3-digit form go--but we're still stuck with it in the axes_grid family, I see, which also has the idiosyncratic nrows_ncols kwarg.

@jklymak jklymak marked this pull request as ready for review May 20, 2020 19:58
@QuLogic
Copy link
Member

QuLogic commented Sep 23, 2020

I don't see any reasons not to merge this, but it's grown a bunch of conflicts now.

@timhoffm
Copy link
Member

👍 on the PR. It's proably easier to redo this from scratch using regex serach&replace than handling all the conflicts.

@anntzer anntzer mentioned this pull request Sep 23, 2020
Closed
@jklymak
Copy link
Member Author

jklymak commented Sep 23, 2020

I only relearn regexp every 6 months or so, so this may have to wait, or anyone else is welcome to take this up instead!

@jklymak jklymak marked this pull request as draft September 23, 2020 20:12
This was referenced Sep 23, 2020
Merged
Merged
@timhoffm
Copy link
Member

timhoffm commented Nov 6, 2020

Most cases have been addressed via the above PRs.

Still open: More complex layouts that are not full single-cell grids. These can be handled using subplot_mosaic, but I'd like to wait until the single-line notation #18763 is in.

@jklymak
Copy link
Member Author

jklymak commented Nov 9, 2020

I'll go ahead and close this, but feel free to restore if you need it to track the issue...

@jklymak jklymak closed this Nov 9, 2020
@jklymak jklymak deleted the doc-soft-deprecate-subplot branch November 9, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer left review comments

Assignees
No one assigned
Labels
Documentation status: waiting for other PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jklymak @timhoffm @matthew-brett @anntzer @efiring @QuLogic