Fix: shifting margins caused by multiple rugplots #2953
Conversation
This fix prevents margins from increasing when multiple rugplots are
added to the same `ax`, even if `expand_margins` is `False`.
As a minimum reproducible example:
```py
import seaborn as sns; sns.set()
import numpy as np
import matplotlib.pyplot as plt
values = np.linspace(start=0, stop=1, num=5)
ax = sns.lineplot(x=values, y=values)
sns.rugplot(x=values, ax=ax)
ylim = ax.get_ylim()
for _ in range(4):
sns.rugplot(x=values, ax=ax, expand_margins=False)
if not all(a == b for a, b in zip(ylim, ax.get_ylim())):
print(f'{ylim} != {ax.get_ylim()}')
plt.suptitle("Example showing that multiple rugplots cause issues")
plt.show()
```
Running the above code:
```sh
$ pip install seaborn numpy matplotlib; python3 test.py
(-0.1, 1.1) != (-0.61051, 1.14641)
```
This bug was caused by how seaborn detects the correct colors to use. In
`seaborn/utils.py`, in method `_default_color`, the following line used
to resolve to `ax.plot([], [], **kws)`:
```py
scout, = method([], [], **kws)
```
But matplotlib has the parameters `scalex` and `scaley` of `ax.plot` set
to `True` by default. Matplotlib would see that the rug was already on
the `ax` from the previous call to `sns.rugplot`, and so it would
rescale the x and y axes. This caused the content of the plot to take up
less and less space, with larger and larger margins as more rugplots
were added.
The fix sets `scalex` and `scaley` both to `False`, since this plot
method is a null-plot and is only used to check what colours should be
used:
```py
scout, = method([], [], scalex=False, scaley=False, **kws)
```
The above line is within an if-elif-else branch, but the other branches
do not suffer this bug and so no change is needed for them.
An additional unit test was also added to catch this bug:
`test_multiple_rugs`.
|
I saw that the unit test doesn't quite fit the format of the others, so let me know if I should change that. I'm happy to do so if it'll leave you with a more consistent code base (: |
There was a problem hiding this comment.
Great sleuthing, and thanks for the fix!
Basic structure of the test looks totally fine, just left a couple of comments about keeping it as simple as possible.
Pleas also add an entry to the v0.12 release notes, e.g. "Fixed a bug where :func:rugplot was ignoring expand_margins=False". This should be categorized as a |Fix|, and please link back to the PR so you get credit!
Codecov Report
@@ Coverage Diff @@
## master #2953 +/- ##
=======================================
Coverage 98.31% 98.31%
=======================================
Files 69 69
Lines 23030 23039 +9
=======================================
+ Hits 22641 22650 +9
Misses 389 389
|
|
And thank you for the entire friggin' library!😁 It's getting late at my local time so I'll respond properly in the morning, but I don't see any remarks that I disagree with. |
|
Hang on, I forgot to remove the import. |
|
There we go, let me know if there are any other changes 😄 As a side note, is there a release date planned for v0.12? I came across this bug while working on a university project so it would be nice to see the update roll out. |
|
Thanks again!
No specific date planned, but I would expect v0.12.0 by the end of the month. |
This fix prevents margins from increasing when multiple rugplots are
added to the same
ax, even ifexpand_marginsisFalse.As a minimum reproducible example (which is similar to the added unit test):
Running the above code:
This bug was caused by how seaborn detects the correct colors to use. In
seaborn/utils.py, in method_default_color, the following line usedto resolve to
ax.plot([], [], **kws):But matplotlib has the parameters
scalexandscaleyofax.plotsetto
Trueby default. Matplotlib would see that the rug was already onthe
axfrom the previous call tosns.rugplot, and so it wouldrescale the x and y axes. This caused the content of the plot to take up
less and less space, with larger and larger margins as more rugplots
were added.
The fix sets
scalexandscaleyboth toFalse, since this plotmethod is a null-plot and is only used to check what colours should be
used:
The above line is within an if-elif-else branch, but the other branches
do not suffer this bug and so no change is needed for them.
An additional unit test was also added to catch this bug:
test_multiple_rugs.