make seaborn great again on Matplotlib-2.2 #10622
Conversation
lib/matplotlib/axes/_subplots.py
Outdated
| 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.
This isn't the same as the old check at all 😉
There was a problem hiding this comment.
hum, result is identical unless one of the 2 sharex/sharey is "self", isn't it ?
lib/matplotlib/axes/_subplots.py
Outdated
| """ | ||
| from matplotlib.projections import process_projection_requirements | ||
| if 'sharex' in kwargs and 'sharey' in kwargs: | ||
| if kwargs["sharex"] is not self and kwargs["sharey"] is not self: |
There was a problem hiding this comment.
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.
It will also give a KeyError if "sharex" and "sharey" aren't in kwargs.
There was a problem hiding this comment.
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... |
efiring
left a comment
There was a problem hiding this comment.
This will be fine once the comment, or a variation of it, is added.
| 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.
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! |
|
Thanks @stonebig |
Backport PR #10622 on branch v2.2.x
fix for #10585
PR Summary
PR Checklist