Skip to content

Changed line color of boxplot for dark_background #15725

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 16, 2020
Merged

Changed line color of boxplot for dark_background #15725

merged 4 commits into from
May 16, 2020

Conversation

manan2501
Copy link
Contributor

@manan2501 manan2501 commented Nov 19, 2019 •
edited
Loading

PR Summary

Rework of #15705. Closes #15675.
Previously, the black outlines of boxes in boxplots were hidden inside the black background in the dark_background style.

Now: Outputs of boxplot_demo.py (https://matplotlib.org/examples/pylab_examples/boxplot_demo.html) using dark_background are as shown below:
image
image
image
image
image
image
image

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@ImportanceOfBeingErnest ImportanceOfBeingErnest changed the title Solved issue #1565 : Changed line color of boxplot for dark_background Changed line color of boxplot for dark_background Nov 19, 2019
@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.3.0 milestone Nov 19, 2019
@ImportanceOfBeingErnest
Copy link
Member

I think the style should only set those properties that are necessary to change for the dark background. E.g. the linewidth does not care if it's a bright or dark background. This would allow to combine different styles.

@manan2501
Copy link
Contributor Author

I think the style should only set those properties that are necessary to change for the dark background. E.g. the linewidth does not care if it's a bright or dark background. This would allow to combine different styles.

I have copied the whole thing from classic.mplstyle and just edited colors.
I haven't touched the linewidth.

@manan2501 manan2501 closed this Nov 19, 2019
@manan2501 manan2501 reopened this Nov 19, 2019
@manan2501
Copy link
Contributor Author

I think the style should only set those properties that are necessary to change for the dark background. E.g. the linewidth does not care if it's a bright or dark background. This would allow to combine different styles.

Do you mean to say linestyle?

@ImportanceOfBeingErnest
Copy link
Member

That's precisely the point. The dark_background style should not contain linewidth, nor any other parameter that is not necessary for dark backgrounds.

@manan2501
### Boxplots boxplot.boxprops.color: w boxplot.capprops.color: w boxp…
96bf24c 
…lot.flierprops.color: w boxplot.flierprops.markeredgecolor: w boxplot.meanprops.color: g boxplot.medianprops.color: g boxplot.meanprops.markerfacecolor: g boxplot.meanprops.markeredgecolor: w boxplot.whiskerprops.color: w
@ImportanceOfBeingErnest
Copy link
Member

Any reason to make the mean and median green instead of the original color from the colorcyle?

@manan2501
Copy link
Contributor Author

Any reason to make the mean and median green instead of the original color from the colorcyle?

I just thought that it would look highlighted. No specific reason.
If it's unappropriate than I will change it?

@manan2501 manan2501 closed this Nov 19, 2019
@manan2501 manan2501 reopened this Nov 19, 2019
timhoffm
timhoffm reviewed Nov 24, 2019
View reviewed changes
timhoffm
timhoffm reviewed Dec 8, 2019
View reviewed changes
lib/matplotlib/mpl-data/stylelib/dark_background.mplstyle Outdated
boxplot.capprops.color: white
boxplot.flierprops.color: white
boxplot.flierprops.markeredgecolor: white
boxplot.meanprops.markeredgecolor: white
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boxplot.meanprops.markerfacecolor and boxplot.meanprops.markeredgecolor are both C2
Therefore, I think we do not need to redefine boxplot.meanprops.markeredgecolor.

tacaswell
tacaswell reviewed May 7, 2020
View reviewed changes
lib/matplotlib/mpl-data/stylelib/dark_background.mplstyle Outdated
Comment on lines 28 to 29
boxplot.flierprops.markeredgecolor: white
boxplot.meanprops.markeredgecolor: white
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
boxplot.flierprops.markeredgecolor: white
boxplot.meanprops.markeredgecolor: white

Per @timhoffm 's suggestion. I think it is better to track the color cycle for these so they stay highlighted from the other lines.

@timhoffm
Copy link
Member

timhoffm commented May 9, 2020

I took the liberty of pushing a commit for not setting boxplot.flierprops.markeredgecolor as discussed above.

timhoffm
timhoffm approved these changes May 9, 2020
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be squashed before/when merging.

@QuLogic
Copy link
Member

QuLogic commented May 16, 2020

Going to ignore doc failures, since they're fixed on master.

@QuLogic QuLogic merged commit 661100a into matplotlib:master May 16, 2020
@story645 story645 mentioned this pull request Aug 10, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Boxplot line color with style dark_background should be bright
5 participants
@manan2501 @ImportanceOfBeingErnest @timhoffm @QuLogic @tacaswell