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

Move impl. of plt.subplots to Figure.add_subplots. #5146

Merged
merged 3 commits into from Nov 22, 2015

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 25, 2015

Also simplify the implementation a bit.

cf. #5139.

Also simplify the implementation a bit.

cf. matplotlib#5139.
@@ -1001,6 +1002,142 @@ def add_subplot(self, *args, **kwargs):
self.stale = True
return a

def add_subplots(self, nrows=1, ncols=1, sharex=False, sharey=False,
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave the name as subplots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit like Figure.suptitle: I don't like it because I don't see why it should be fig.add_subplot but fig.subplots, just like I don't see why it should be ax.set_title but fig.suptitle.

Copy link
Member

Choose a reason for hiding this comment

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

but, the name subplots has been around for a while now and it is better to not rename functionality if we don't have to. From the point of view of the user moving from pyplot -> OO you don't want to make them re-learn the names of things.

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

Please don't merge this until we get 1.5 on it's own branch.

Returns
-------
ax : single Axes object or array of Axes objects
The addes axes. The dimensions of the resulting array can be
Copy link
Member

Choose a reason for hiding this comment

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

typo -> 'adds'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@tacaswell
Copy link
Member

This is still waiting on a few typos and a function re-name.

I am sold on not calling clf.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 12, 2015

I was just waiting for the decision on whether to call clf.

array:

- if only one subplot is constructed (nrows=ncols=1), the resulting
single Axes object is returned as a scalar.
Copy link
Member

Choose a reason for hiding this comment

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

The docs build fails because this needs to be indented to be valid RST i.e.

- if only one subplot is constructed (nrows=ncols=1), the resulting
  single Axes object is returned as a scalar.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 12, 2015

Fixed.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 21, 2015

Kindly bumping the issue.

@jenshnielsen
Copy link
Member

Im 👍 on merging this now. Only questing is does this interfere with any of your work @OceanWolf and @fariza

@OceanWolf
Copy link
Contributor

No, I don't see that it does, AFAIK this only touches Figure, we only deal with FigureManager.

@cimarronm
Copy link
Contributor

👍 LGTM

@OceanWolf
Copy link
Contributor

One thing I do note, this PR only deals with the pyplot.subplots function, but I wonder if we should not also do the same to the pyplot.subplot2grid function? If we do that as well we can get rid of the GridSpec import in pyplot. Thoughts?

@@ -1131,106 +1131,11 @@ def subplots(nrows=1, ncols=1, sharex=False, sharey=False, squeeze=True,
# same as
plt.subplots(2, 2, sharex=True, sharey=True)
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we eliminate a lot of this docstring? Either with @_autogen_docstring or %(Figure.subplots)s or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to make this work, given that the APIs are subtly different (one returns just the new axes, the other returns a figure, axes pair). Open to suggestions...

Copy link
Member

Choose a reason for hiding this comment

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

Not super worried about the duplication of docstrings at this point. An interesting follow on to this PR would be to generate plt.subplots() via boilerplate.py.

tacaswell added a commit that referenced this pull request Nov 22, 2015
ENH: Move impl. of plt.subplots to Figure.add_subplots.

close #5139
@tacaswell tacaswell merged commit 91f58db into matplotlib:master Nov 22, 2015
@tacaswell
Copy link
Member

@anntzer Thanks! This is a major step towards moving away from pyplot.

@anntzer anntzer deleted the figure-subplots branch June 1, 2016 18:16
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

6 participants