Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
make seaborn great again on Matplotlib-2.2#10622
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lib/matplotlib/axes/_subplots.py Outdated
| @@ -172,8 +172,8 @@ def _make_twin_axes(self, *kl, **kwargs): | |||
| Make a twinx axes of self. This is used for twinx and twiny. | |||
| """ | |||
| from matplotlib.projections import process_projection_requirements | |||
| if 'sharex' in kwargs and 'sharey' in kwargs: | |||
| raise ValueError("Twinned Axes may share only one axis.") | |||
| if kwargs["sharex"] is not self and kwargs["sharey"] is not self: | |||
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.
This isn't the same as the old check at all 😉
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.
hum, result is identical unless one of the 2 sharex/sharey is "self", isn't it ?
lib/matplotlib/axes/_subplots.py Outdated
| @@ -172,7 +172,7 @@ def _make_twin_axes(self, *kl, **kwargs): | |||
| Make a twinx axes of self. This is used for twinx and twiny. | |||
| """ | |||
| from matplotlib.projections import process_projection_requirements | |||
| if'sharex' in kwargsand'sharey' in kwargs: | |||
| ifkwargs["sharex"] is not selfandkwargs["sharey"] is not self: | |||
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.
I think this needs to be a separate check as well as the one it has replaced right? The currently one checks that both x and y aren't shared at the same time, and you want to check that we're not trying to share an axis with itself.
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 will also give aKeyError if "sharex" and "sharey" aren't inkwargs.
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.
argh... ok, tuning
This looks better, but lets see what happens to the tests.
Asking@efiring to look this over. It passes all the tests, so I guess its OK. Tempted to ask for a test, but since tests shouldn't particularly call the internal API.... |
In this case it's entirely appropriate for the test to call the internal API IMO. |
Actually, I take that back. No tests necessary, because they're going to come with the PR for the public API that someone from seaborn is going to submit. |
See#9923 for a proposed public API that I think addresses Seaborn's use case... |
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.
This will be fine once the comment, or a variation of it, is added.
lib/matplotlib/axes/_subplots.py Outdated
| @@ -173,7 +173,8 @@ def _make_twin_axes(self, *kl, **kwargs): | |||
| """ | |||
| from matplotlib.projections import process_projection_requirements | |||
| if 'sharex' in kwargs and 'sharey' in kwargs: | |||
| raise ValueError("Twinned Axes may share only one axis.") | |||
| if kwargs["sharex"] is not self and kwargs["sharey"] is not self: | |||
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.
Please add a comment above this line:
# The following line is added in v2.2 to avoid breaking Seaborn,# which currently uses this internal API.PEP8 is a little silly about trailing white space... |
"PEP8 is a little silly about trailing white space..." Not silly at all--trailing whitespace is an abomination! Much of the rest of what the automated checker complains about verges on "silly", but not the trailing whitespace! |
mwaskom commentedMar 1, 2018
Thanks@stonebig |
Backport PR#10622 on branch v2.2.x
fix for#10585
PR Summary
PR Checklist