Let boxplot() defer rcParams application to bxp() #13449
Merged
Conversation
This file contains hidden or 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
instead of duplicating the defaults-applying code in both methods. This caught an oversight in the application of the boxplot.boxprops.linewidth rcParam in bxp (see changelog note); because of that, the bxp testing code needed some update (to keep the old behavior). Preliminary work to having bxp() normalize properties passed in the boxprops, meanprops, etc. kwargs as well.
8c17fd7 to
47d75f2
Compare
phobson
reviewed
Feb 18, 2019
lib/matplotlib/axes/_axes.py
Outdated
| markersize=rcParams['boxplot.flierprops.markersize'], | ||
| ) | ||
|
|
||
| # cap properties |
There was a problem hiding this comment.
For me, it takes enough mental bandwidth to read these dict-comprehensions that I wonder if factoring out into a sub function makes sense
There was a problem hiding this comment.
pushed a second commit that does it, indeed that's a good idea.
phobson
approved these changes
Feb 18, 2019
| @@ -2108,68 +2108,59 @@ def layers(n, m): | ||
| axs[1, 1].stackplot(range(100), d.T, baseline='weighted_wiggle') | ||
|
|
||
|
|
||
| def _bxp_test_helper( | ||
| stats_kwargs={}, transform_stats=lambda s: s, bxp_kwargs={}): |
There was a problem hiding this comment.
this isn't a big deal. feel free to ignore it. but my early morning, uncaffeinated brain has a hard time with function definitions whose first parameters is on a second line
jklymak
reviewed
Feb 19, 2019
| ax.set_xscale('log') | ||
| # Work around baseline images generate back when bxp did not respect the | ||
| # boxplot.boxprops.linewidth rcParam when patch_artist is False. | ||
| if not bxp_kwargs.get('patch_artist', False): |
There was a problem hiding this comment.
OK, but you never test the new proper behaviour this way, do you?
There was a problem hiding this comment.
indeed. slightly changed the test setup to test that the rc is used.
84bdc71 to
2b8aee2
Compare
jklymak
approved these changes
Feb 22, 2019
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.
instead of duplicating the defaults-applying code in both methods.
This caught an oversight in the application of the
boxplot.boxprops.linewidth rcParam in bxp (see changelog note);
because of that, the bxp testing code needed some update (to keep the
old behavior).
Preliminary work to having bxp() normalize properties passed in the
boxprops, meanprops, etc. kwargs as well (#13448).
PR Summary
PR Checklist