FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

make seaborn great again on Matplotlib-2.2 by stonebig · Pull Request #10622 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

make seaborn great again on Matplotlib-2.2 - #10622

Merged
jklymak merged 5 commits into
matplotlib:masterfrom
stonebig:patch-1
Feb 28, 2018
ConversationCommits (5)ChecksFiles changed
Merged

make seaborn great again on Matplotlib-2.2#10622
jklymak merged 5 commits into
matplotlib:masterfrom
stonebig:patch-1

Conversation

stonebig commented Feb 27, 2018

Copy link
Copy Markdown
Contributor

fix for #10585

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak previously requested changes Feb 27, 2018
View reviewed changes
Comment thread 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:

jklymak Feb 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This isn't the same as the old check at all 😉

stonebig Feb 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

hum, result is identical unless one of the 2 sharex/sharey is "self", isn't it ?

dstansby previously requested changes Feb 27, 2018
View reviewed changes
Comment thread 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:

dstansby Feb 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

dopplershift Feb 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It will also give a KeyError if "sharex" and "sharey" aren't in kwargs.

stonebig Feb 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

argh... ok, tuning

tacaswell added this to the v2.2.0 milestone Feb 27, 2018
jklymak dismissed their stale review February 27, 2018 22:41

This looks better, but lets see what happens to the tests.

jklymak requested a review from efiring February 27, 2018 23:57

jklymak commented Feb 28, 2018

Copy link
Copy Markdown
Member

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....

dopplershift commented Feb 28, 2018

Copy link
Copy Markdown
Contributor

In this case it's entirely appropriate for the test to call the internal API IMO.

dopplershift commented Feb 28, 2018

Copy link
Copy Markdown
Contributor

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.

jklymak commented Feb 28, 2018

Copy link
Copy Markdown
Member

See #9923 for a proposed public API that I think addresses Seaborn's use case...

dopplershift mentioned this pull request Feb 28, 2018
Closed
6 tasks done
dstansby dismissed their stale review February 28, 2018 20:18

Issue solved

efiring requested changes Feb 28, 2018
View reviewed changes

efiring left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This will be fine once the comment, or a variation of it, is added.

Comment thread lib/matplotlib/axes/_subplots.py
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:

efiring Feb 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jklymak commented Feb 28, 2018

Copy link
Copy Markdown
Member

PEP8 is a little silly about trailing white space...

pep8
09a17f4 
remove trailing spaces on a comment line

efiring commented Feb 28, 2018

Copy link
Copy Markdown
Member

"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!

jklymak merged commit d2e68ff into matplotlib:master Feb 28, 2018
lumberbot-app Bot pushed a commit that referenced this pull request Feb 28, 2018
Backport PR #10622: make seaborn great again on Matplotlib-2.2
48a74a0
lumberbot-app Bot mentioned this pull request Feb 28, 2018
Merged
jklymak mentioned this pull request Feb 28, 2018
Closed

mwaskom commented Mar 1, 2018

Copy link
Copy Markdown

Thanks @stonebig

This was referenced Mar 1, 2018
Closed
Closed
jklymak added a commit that referenced this pull request Mar 1, 2018
Merge pull request #10634 from matplotlib/auto-backport-of-pr-10622
39e9773 
Backport PR #10622 on branch v2.2.x
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

efiring efiring approved these changes
dopplershift dopplershift approved these changes
jklymak jklymak left review comments
dstansby dstansby left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.2.0

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL