Add separate keyword argument dictionaries for components of boxenplot #2909
Conversation
This file contains 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
Removed `kwargs` which were used to draw the median lines and scatter plot of outliers previously. Added separate kwargs - `box_kws`, `line_kws` (drawing the median lines) and `flier_kws` (for the scatter of outliers). Updated the matching docstring.
…reformatted. Here it is reverted and only the changes to `seaborn.categorical.boxenplot` and `seaborn.categorical._LVPlotter` are kept.
mwaskom
requested changes
Jul 19, 2022
There was a problem hiding this comment.
Thanks @EitanHemed for taking a crack at this. The logic looks right to me; I have a number of comments that are mostly cosmetic.
The comments about the boxenplot docstring are critical though and I am wondering how those changes ended up in here ... are you using some kind of automated formatting tool that introduced them? If so, that tools seems broken.
More importantly, could you please add a test to cover this new behavior? Thanks.
seaborn/categorical.py
Outdated
Comment on lines
1962
to
1968
| # Set the default kwargs for the boxes | ||
| box_default_kws = dict(cmap=cmap, | ||
| edgecolor=self.gray, | ||
| linewidth=self.linewidth) | ||
|
|
||
| for k, v in box_default_kws.items(): | ||
| box_kws.setdefault(k, v) |
There was a problem hiding this comment.
I would move this up to where you handle the other defaults / custom kwargs.
There was a problem hiding this comment.
I moved most of it upwards, but kept the cmap argument below, as it is only being assigned on this part of the function.
cmap itself cannot be easily moved upwards, as it relies on rgb, which relies on hex_color.
mwaskom
changed the title
mwaskom
reviewed
Jul 19, 2022
mwaskom
reviewed
Jul 19, 2022
mwaskom
reviewed
Jul 19, 2022
mwaskom
reviewed
Jul 19, 2022
mwaskom
reviewed
Jul 19, 2022
- Removed commented out code in the `plot` function
Disentangled the nested for loop for default kwargs assignment
…only a single data point.
Two current issues (a) most are not testing when multiple categories are used on the x-axis, but only a single one. (b) the tests for the box_kws functionality are very slim.
mwaskom
added a commit
that referenced
this pull request
Aug 13, 2022
* Sorting boxenplot * Boxenplot separate kws Removed `kwargs` which were used to draw the median lines and scatter plot of outliers previously. Added separate kwargs - `box_kws`, `line_kws` (drawing the median lines) and `flier_kws` (for the scatter of outliers). Updated the matching docstring. * In the previous commit most code on the categorical.py file was auto-reformatted. Here it is reverted and only the changes to `seaborn.categorical.boxenplot` and `seaborn.categorical._LVPlotter` are kept. * Reinserted blank lines in docstring. * - Removed redundant indention in `boxenplot` function - Removed commented out code in the `plot` function * Removed default kwargs from `plot` * Removing commented out code * Reverted to ternary expressions * Replaced default kwargs assignment to box_kws Disentangled the nested for loop for default kwargs assignment * Removed remaining `kwargs` item in docstring * Resolved incorrect reference in the box_kws item on the docstring. * Resolved incorrect descriptions for box_kws, line_kws and flier_kws. * Changed line_kws update to source arguments frmo box_kws if there is only a single data point. * Added line_kws test * Added flier_kws test, renamed line_kws test * Tests - further work is required in expanding the tests. Two current issues (a) most are not testing when multiple categories are used on the x-axis, but only a single one. (b) the tests for the box_kws functionality are very slim. * Fix lint issues * Fix pinned tests * Update release notes * Cleanup boxenplot colors test Co-authored-by: EitanHemed <[email protected]>
|
Thanks @EitanHemed I took care of the final touches (fixing the lint errors and test failures on pinned versions) so that this could make it into the release candidate. |
Closed
|
@mwaskom great, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Aiming to solve issue #2701 - adding separate keyword support for
boxenplot.Changes -
Removed
kwargswhich were used to draw the median lines and scatter plot of outliers previously.Added separate kwargs -
box_kws,line_kws(drawing the median lines) andflier_kws(for the scatter of outliers).Updated the matching docstring.
Passed all relevant tests.
Closes #2701