-
Notifications
You must be signed in to change notification settings - Fork 2k
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
JointGrid reference lines #2620
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2620 +/- ##
=======================================
Coverage 97.44% 97.45%
=======================================
Files 17 17
Lines 6350 6371 +21
=======================================
+ Hits 6188 6209 +21
Misses 162 162
Continue to review full report at Codecov.
|
Now both # x and y missing -- nothing happens
g.refline(linestyle='dashed', color='gray')
# marginal and joint are False -- nothing happens
g.refline(x=45, y=16, marginal=False, joint=False, linestyle='dashed', color='gray') Passing both g.refline(x=45, y=16, linestyle='dashed', color='gray') |
Great! So, three thoughts now:
|
|
Given that the defaults will differ a bit from the analagous matplotlib functions, I think it would be good to have them explicit in the signature. (But the code will be cleaner if you inject them into the
I don't think allowing multiple x/y's is worth the code complexity / thinking about what that API would look like. Let's have |
I've added 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.
One tiny nit in the docstring, otherwise looks perfect!
Everything looks great so far! Once implemented in |
Co-authored-by: Michael Waskom <[email protected]>
Can you confirm that the kde_ridgeplot example is running correctly for you? When I run it (without switching to |
Ah, interesting! That does seem to be broken on master (not on v0.11.1). |
Probably the result of this: #2583. "Improvement", indeed... |
OK thinking about this a bit more, I think that is the desired result of #2583. Using a more standard case: sns.FacetGrid(tips, col="time").map(sns.kdeplot, "total_bill") v0.11.1: master: I think master is right (or better) here, It produces an undesirable result in the ridgeplot example because lots of additional tweaking has been done to the plot, but that's ok, all that needs to be done is to add |
For the release notes, how should this be tagged? I took a look at the release notes for v0.11.0, which also has some new entries for new methods. Those are tagged "Feature", but since this PR is tagged "Enhancement", I figured I would double check. |
Hm good question. Actually there's no feature label on GitHub 🙃. I think the feature/enhancement boundary can be a little blurry (is this adding a wholly new feature, or enhancing the existing JointGrid/FacetGrid functionality?) but I think Feature probably fits best. |
Agreed. I'll push the rest of the changes up shortly. |
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.
A couple of tiny comments, otherwise I think this looks great!
Thanks @stefmolin! |
* Add method to JointGrid for plotting reference lines. * Tweak docstring * Require one of joint/marginal to be True. * Switch from orient to x/y. * Add additional input validation. * Allow both x and y; remove ValueErrors. * Add color and linestyle params. * Add JointGrid.refline() tests. * Add JointGrid.refline() example. * Update JointGrid.refline() docstring Co-authored-by: Michael Waskom <[email protected]> * Add FacetGrid.refline() * Switch examples to use FacetGrid.refline() * Use :meth: for method reference. * Add FacetGrid.refline() test and example. * Add refline to release notes. * Address PR comments Co-authored-by: Michael Waskom <[email protected]> (cherry picked from commit 70cb3d9)
Closes #2249 – Adds new
JointGrid.refline()
method:Example usage:
Needs: