-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Move impl. of plt.subplots to Figure.add_subplots. #5146
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
Conversation
Also simplify the implementation a bit. cf. matplotlib#5139.
lib/matplotlib/figure.py
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Please don't merge this until we get 1.5 on it's own branch. |
lib/matplotlib/figure.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo -> 'adds'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
This is still waiting on a few typos and a function re-name. I am sold on not calling |
|
I was just waiting for the decision on whether to call |
lib/matplotlib/figure.py
Outdated
There was a problem hiding this comment.
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.
92a386c to
eded075
Compare
|
Fixed. |
|
Kindly bumping the issue. |
|
Im 👍 on merging this now. Only questing is does this interfere with any of your work @OceanWolf and @fariza |
|
No, I don't see that it does, AFAIK this only touches |
|
👍 LGTM |
|
One thing I do note, this PR only deals with the |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
ENH: Move impl. of plt.subplots to Figure.add_subplots. close #5139
|
@anntzer Thanks! This is a major step towards moving away from |
Also simplify the implementation a bit.
cf. #5139.