Rejecting parameter 'positions' for boxplot(), refs #3566 #3576
Conversation
There was a problem hiding this comment.
Thanks, I think raising an error here makes sense; it looks like positions errored on <=0.12 anyway since seaborn was already passing positions directly. (In 0.13, there was an effort to allow users to override more of the matplotlib params that seaborn sets defaults for, though as you discovered in the original issue, there's no guarantee that the overridden behavior will be correct).
seaborn/categorical.py
Outdated
| @@ -1577,6 +1577,10 @@ def boxplot( | |||
| fliersize=None, hue_norm=None, native_scale=False, log_scale=None, formatter=None, | |||
| legend="auto", ax=None, **kwargs | |||
| ): | |||
|
|
|||
| if "positions" in kwargs: | |||
| msg = "boxplot() does not support parameter 'positions'. Consider to use native_scale=True and specify positions via parameter 'x'." | |||
There was a problem hiding this comment.
| msg = "boxplot() does not support parameter 'positions'. Consider to use native_scale=True and specify positions via parameter 'x'." | |
| msg = "boxplot does not support parameter `positions`. Consider using `native_scale=True`. |
Nit: For a horizontal boxplot you'd specify the positions with y so let's make the suggestion less specific. Note that I've also tweaked the formatting.
seaborn/categorical.py
Outdated
|
|
||
| if "positions" in kwargs: | ||
| msg = "boxplot() does not support parameter 'positions'. Consider to use native_scale=True and specify positions via parameter 'x'." | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Small thing but TypeError is probably a better error class here, that is what you get if you passed an undefined parameter. Alternatively, RuntimeError since positions is technically valid but produces incorrect behavior.
tests/test_categorical.py
Outdated
| try: | ||
| boxplot(**kwargs, positions=positions) | ||
| err = None | ||
| except ValueError as e: | ||
| err = e | ||
|
|
||
| assert(err is not None) |
There was a problem hiding this comment.
It would be slightly more idiomatic I think to use pytest.raises here, ideally with the match parameter to make sure we're getting the error we expect.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3576 +/- ##
==========================================
- Coverage 98.58% 98.34% -0.25%
==========================================
Files 75 75
Lines 24706 24641 -65
==========================================
- Hits 24357 24233 -124
- Misses 349 408 +59
|
|
Hi, I came back to this now and updated my branch. How do I proceed? Should I open a new PR or can this be updated somehow? Sorry, I'm not very familiar with this process... :) |
Hi, this is a first attempt for the boxplot position rejection (#3566). I'm not sure about the style. Please review and let me know if something should be changed.
Cheers