Skip to content
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

Plotting seaborn.FacetGrid compatibility #314

Merged
merged 19 commits into from
Nov 26, 2022

Conversation

remrama
Copy link
Contributor

@remrama remrama commented Nov 25, 2022

This addresses a feature request in #306 to offer more flexibility in pingouin's plotting module. The changes were targeted at making some of the functions useable with the g.map_dataframe() method attached to seaborn grids, but they offer more flexibility when used alone as well.

The main things:

  • Removed all fig and dpi keyword arguments (users should be advised to generate their own figures first, eg with plt.subplot and then pass the ax to pingouin)
  • Removed the plot_blandaltmann keyword argument scatter_kws and now any kwarg gets passed through. This was done because gmap_dataframe requires the passing of a color keyword (and label), and it can be useful at times.
  • Note that now, without passing fig through, the default axis that pops up is not always square. So for the few plotting functions that probably should be square (qqplot, circmean), I added an ax.set_aspect("equal") in the docstring examples.

If you want to see some arbitrary examples of the use-case I'm talking about:

import pingouin as pg
import seaborn as sns
import matplotlib.pyplot as plt

iris = sns.load_dataset("iris")

g = sns.FacetGrid(iris, col="species", aspect=1)
g.map(pg.qqplot, "petal_width")
plt.show()

g = sns.FacetGrid(iris, col="species", hue="species", sharey=False)
g.map(pg.plot_blandaltman, "petal_width", "sepal_width")
plt.show()

@raphaelvallat
Copy link
Owner

raphaelvallat commented Nov 25, 2022

Hi @remrama,

Thanks for the PR, great idea! A few comments:

  • Can you please update the unit tests?
  • Update the changelog
  • Does this require to bump seaborn's version in the requirements/setup?
  • I do feel thatax.set_aspect("equal") should be the default for qqplot and circmean. Is there a way that we could ensure that, e.g. by including a square=True parameter like in seaborn.heatmap?

@raphaelvallat
Copy link
Owner

@remrama I've rebased the PR to master, make sure you git pull before making new changes!

@codecov
Copy link

codecov bot commented Nov 26, 2022

Codecov Report

Base: 98.67% // Head: 98.58% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (56f7924) compared to base (6816d02).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
- Coverage   98.67%   98.58%   -0.09%     
==========================================
  Files          18       18              
  Lines        3312     3320       +8     
  Branches      537      540       +3     
==========================================
+ Hits         3268     3273       +5     
  Misses         25       25              
- Partials       19       22       +3     
Impacted Files Coverage Δ
pingouin/plotting.py 95.72% <83.33%> (-0.78%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@remrama
Copy link
Contributor Author

remrama commented Nov 26, 2022

@raphaelvallat I think this is good to go?

It passes unittests, changelog is updated, and square keyword arguments were added to qqplot() and plot_circmean(). Nothing to worry about for seaborn version requirements. map() was present in version 0.10, and you already require 0.11.

:py:method: not working + add ptests to API
@raphaelvallat
Copy link
Owner

@remrama just made some very minor edits to documentation. Merging when the CIs are complete

@raphaelvallat raphaelvallat merged commit 1f329ef into raphaelvallat:master Nov 26, 2022
@remrama remrama deleted the plotting_free_the_figs branch November 26, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants