Navigation Menu

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

use plt.subplots() in examples as much as possible #1462

Closed
wants to merge 4 commits into from

Conversation

ivanov
Copy link
Member

@ivanov ivanov commented Nov 8, 2012

This is a docs-only update - but I think it'll be worthwhile to start to give a more sensible way of creating figures and axes in one call for our the folks who'll be looking at matplotlib with fresh eyes after the 1.2 release.

But I totally understand if folks feel this is too big of a pill to swallow this late in the game.

At the recent LBL Software Carpentry Workshop, it was pointed out that there's
an inconsistency within our documentation for how to create new figures with
subplots.

Indeed, most examples were using the old way, something like:

fig = plt.figure()
ax = plt.subplot(111) # or plt.add_subplot(111)

This patch changes a whole bunch of instances like the above to:

fig, ax = plt.subplots()

We should strive to have a minimal amount of constants in our code,
especially unusual ones like 111, which only make sense to Matlab
refugees.

I have left unchanged examples which were using axes keywords passed to
subplot() and add_subplot(), since those end up transforming things like:

figure()
subplot(111, axisbg='w')

to

plt.subplots(subplot_kw=dict(axisbg='w'))

which isn't necessarily better.

I also did not touch most of the user_interfaces examples, since those did not
involve using plt, but instead explicitly imported Figure, and used the OO
approach on Figure instances.

Also updated instaces where the old "import pylab as p" convention was used to
use our standard "import matplotlib.pyplot as plt"

I have also updated some, but not all uses of subplot(121) etc, but I'm a bit
exhausted after doing all of these.

At the recent LBL Software Carpentry Workshop, it was pointed out that there's
an inconsistency within our documentation for how to create new figures with
subplots.

Indeed, most examples were using the old way, something like:

    fig = plt.figure()
    ax = plt.subplot(111) # or plt.add_subplot(111)

This patch changes a whole bunch of instances like the above to:

    fig, ax = plt.subplots()

We should strive to have a minimal amount of constants in our code,
especially unusual ones like `111`, which only make sense to Matlab
refugees.

I have left unchanged examples which were using axes keywords passed to
subplot() and add_subplot(), since those end up transforming things like:

    figure()
    subplot(111, axisbg='w')

to

    plt.subplots(subplot_kw=dict(axisbg='w'))

which isn't necessarily better.

I also did not touch most of the user_interfaces examples, since those did not
involve using plt, but instead explicitly imported Figure, and used the OO
approach on Figure instances.

Also updated instaces where the old "import pylab as p" convention was used to
use our standard "import matplotlib.pyplot as plt"

I have also updated some, but not all uses of subplot(121) etc, but I'm a bit
exhausted after doing all of these.
@@ -24,7 +24,7 @@ class BlitQT(QObject):
def __init__(self):
QObject.__init__(self, None, "app")

self.ax = p.subplot(111)
fig, self.ax = plt.subplots()
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether:

fig, ax = plt.subplots()

is better than

ax = plt.subplot(111)

or even ax = plt.axes()

Note:

I do agree that:

fig, self.ax = plt.subplots()

is better than

fig = plt.figure()
ax = plt.subplot(111)

Copy link
Member Author

Choose a reason for hiding this comment

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

this inspired me to make #1475 to have plt.subplot() work just like plt.axes().

I'm happy to change this to just be plt.axes()

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Glad it inspired you! 😄

I think we can use your new syntax here now. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

On Tue, Nov 13, 2012 at 1:37 AM, Phil Elson notifications@github.comwrote:

Cool. Glad it inspired you! [image: 😄]

I think we can use your new syntax here now. Thoughts?

This is going into 1.2.x and the new feature was merged into master, so we
could only change this once it's merge in master.

This is the sort of bifurcation that I was talking about in the recent
thread on mpl-devel - but no solution is perfect...

@mdboom
Copy link
Member

mdboom commented Nov 8, 2012

I think this is fine to go on the 1.2.x branch, but as the 1.2.0 release is pending today, let's hold off on merging this until after 1.2.0 is tagged.

@pelson
Copy link
Member

pelson commented Nov 8, 2012

let's hold off on merging this until after 1.2.0 is tagged.

The only problem with that is, if we miss a complete clanger in v1.2.0 we will want to release a v1.2.1 pretty quickly, and that will end up being a bigger change than we would ideally like.

Just wanted to make that issue clear - I still agree with your suggestion though @mdboom .

@dmcdougall
Copy link
Member

Just as a note, it might be the case e12a16e will need to be changed if #1457 is accepted.

Edit: Not in this pull request, though.

@ivanov
Copy link
Member Author

ivanov commented Nov 10, 2012

ok, should be all good to go now, I went and flaked out some more unused modules, and found a couple of places where I had missed the fact that plt had not been imported, so plt.subplots was not going to work.

@dmcdougall
Copy link
Member

Personally, I feel this is a rather big change. I also feel this isn't really a bugfix and, as a result, should not be merged into v1.2.x. I understand others feel differently, so I'm happy to back down of the consensus is to merge into the maintenance branch.

@WeatherGod
Copy link
Member

I agree with @dmcdougall. This is much better positioned to go into master, not maintenance. I hadn't realized at first that this was set to go into v1.2.x.

@ivanov
Copy link
Member Author

ivanov commented Nov 13, 2012

I'm fine either way, I originally submitted this against master in #1458 for that reason, so I can reopen that and update it with the changes I've added here so far.

The argument for having it go into v1.2.x is that there are no functional changes here - the only change that's not in one of the examples is a simple addition of the description of subplots() function to the docstring of pylab.py - so no one's code will be broken as a result of this - and this is just a documentation update.

If this doesn't make it into v1.2.x, should there be 1.2.x releases, all it would mean is that our docs will stay "uglier" for longer. I can live with that, but that's essentially what's at stake here.

@ivanov
Copy link
Member Author

ivanov commented Nov 26, 2012

just pinging everyone for a second opinion: as I understand it, @mdboom, @pelson and I are all ok with these documentation-only changes going into v1.2.x. @dmcdougall and @WeatherGod are against it.

I consider this to be fix for a bug in our docs, and would prefer user-facing docs to get this facelift sooner, rather than later.

@pelson
Copy link
Member

pelson commented Nov 26, 2012

as I understand it, @mdboom, @pelson and I are all ok with these

Yep. Though a part of me says "don't bother" as I don't really want to see a v1.2.1 release, as I'd rather go straight to a v1.3 😄

@dmcdougall
Copy link
Member

Sorry for going incognito everyone. I just started a new job in a different country and I've been snowed under with admin and work.

I'd also rather see a v1.3, but I guess the final call goes to @mdboom.

Also, don't let my opinion hold this up. If the consensus has one opinion and I have another, I am happy to agree.

@mdboom
Copy link
Member

mdboom commented Nov 26, 2012

Yeah -- I think I'm coming down on the side of this being better on master after all. This is changing a lot of code after all, even if it is only example code. I think we should be conservative here.

@ivanov ivanov closed this Dec 8, 2012
@ivanov ivanov mentioned this pull request Dec 14, 2012
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.

None yet

5 participants