FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

style: make grid more discreet in dark_background by ubitux · Pull Request #23598 · matplotlib/matplotlib · GitHub

Repository navigation

style: make grid more discreet in dark_background - #23598

Closed
ubitux wants to merge 1 commit into
matplotlib:mainfrom
ubitux:nicer-dark-grid
Closed

ubitux wants to merge 1 commit into
matplotlib:mainfrom
ubitux:nicer-dark-grid

Conversation

ubitux commented Aug 10, 2022

Copy link
Copy Markdown

PR Summary

Currently:

Proposed:

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

story645 added topic: styles API: default changes Changes to default behavior labels Aug 10, 2022

story645 commented Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

thanks for the PR! I think this is a reasonable change (though I'd prefer the grid a touch darker) given that in light mode the grid is lighter than the axes and this style is supposed to be analogous.

github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

ubitux commented Aug 10, 2022

Copy link
Copy Markdown
Author

thanks for the PR! I think this is a reasonable change (though I'd prefer the grid a touch darker) given that in light mode the grid is lighter than the axes and this style is supposed to be analogous.

Even darker? Did you mean lighter? Something like 777777 maybe?

story645 commented Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Even darker? Did you mean lighter? Something like 777777 maybe?

Yeah, sorry meant brighter/closer to white, something in the .7-.9 range

ubitux commented Aug 10, 2022

Copy link
Copy Markdown
Author

Even darker? Did you mean lighter? Something like 777777 maybe?

Yeah, sorry meant brighter/closer to white, something in the .7-.9 range

This is going to be way too light. They eye is more sensitive to nuances in the dark colors, and you have to take into account gamma compression, so you can't just be symmetric to the light colors mathematically. Also, color models are extremely complex, you're better off choosing with your eyes. For the record, here is with bbbbbb (73%):

And here is 777777:

jklymak commented Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Is this a proposed new style? I don't think we change the existing styles for compatibility reasons.

ubitux commented Aug 10, 2022

Copy link
Copy Markdown
Author

Is this a proposed new style? I don't think we change the existing styles for compatibility reasons.

No it's simply adjusting the grid color for the dark_background style; I have a hard time believing people appreciate the current grid color since enabling it tends to make the graph unreadable.

@story645 do you want me to update the commit to switch to 777777?

Also, is the CI supposed to pass currently?

jklymak commented Aug 10, 2022

Copy link
Copy Markdown
Member

Like it or not the current style is the current style and it's hard to know whether anyone likes it or not. But our policy is pretty clear that we won't change existing styles and we are extremely parsimonious about adding new styles.

jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can't just change an existing style.

ubitux commented Aug 10, 2022

Copy link
Copy Markdown
Author

OK too bad; If we are to add a new theme, I'd rather re-do it entirely, but that's going to be too much work for me so I'll pass. I'll keep this as a local change for now and hope for the best with regards to #23599

ubitux closed this Aug 10, 2022

story645 commented Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

@story645 do you want me to update the commit to switch to 777777?

Yes

@jklymak if we can't change the existing style*, then I think in this case it's very much worth it to add a new dark style with lighter gridlines. I think the gridlines being white in the current dark style is kind of a bug since that behavior is inconsistent with the default behavior on the light theme - ie in the light theme, the gridlines are not the same color as the axes.

*eta: and while yes we're conservative, I can't find a documented policy

ubitux reopened this Aug 10, 2022

ubitux commented Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Here are the default and different flavors:

Edit: BTW, is there a way to make the x and y axis passing through 0 thicker/brighter?

Copy link
Copy Markdown
Member

I think 7 and 6 are great and also going by the documentation on the dark background, I'm really not sure that white gridlines for dark gray lines was intentional.
This

# Set black background default line colors to white.

sounds like the intent was black->white and the gridlines got bundled in

QuLogic added the status: needs comment/discussion needs consensus on next step label Aug 10, 2022

Copy link
Copy Markdown
Member

It is our long standing position that because we are a plotting library the visual output is part of the API and hence changes to the style is an API change. This is reflected in the test suite where we do exact image comparisons for many of the tests. This is also the reason why we did v2.0 for changing the default styles and our "seaborn" styles no longer actually match seaborn (because seaborn changed and we did not follow). In many ways changing the visual appearance is a much more insidious change than a change that breaks user code. At least if the code raises the user knows something in broken, if we have "just" changed the styling everything works, but the output is changed!

We also have a soft moratorium on new styles and color maps due to this. We can not easily change them after they are released and do not want to end up with a growing number of subtle variations.

I am not sure how you are making the local changes, but

mpl.style.use(['dark_background', {'grid.color': '#777'}])

works to easily tweak a style (or even combine multiple styles!).


Thank you for your work on this @ubitux , however I think it is very unlikely we will take a change to the style at this time. I hope you are not discouraged and we hear from you again!

I am undecided on if we should add more dark style files, but if we do it should be more substantial than just changing the grid color.


As an unrelated side point, circleCI builds from the tip of your branch not from the implicit merge commit. I think this is branched from a very old commit so you are seeing a very old (and now bit-rotted) circleci config. Rebasing on main would fix the docs build.

tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As discussed in a comment, changes to styles are API changes.

story645 commented Aug 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

So the context for this PR was @ubitux made a comment on twitter about the grid (link) so I responded open an issue/PR because far as I know that's the route folks are supposed to take to raise suggestions. @ubitux then had this PR in within 30 minutes, which I think is amazing.

I understand that this is an API change, which is why I labeled it as such, but we merge the occasional API change when warranted. I think here grids that are not visually distinct from the axis are generally unwanted, which I think is supported by the matplotlib docs:

I'm honestly unsure here if the grid is a different color or appears as such because it is so much thinner than the axis https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/mpl-data/stylelib/classic.mplstyle#L279, but that is the sort of perceptual issue where a 1-1 approach might not work, as mentioned in #23598 (comment)

timhoffm commented Aug 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

IMHO we need to become better at theme-management

  • We need stability guarantees for those who need them.
  • We need a theme update strategy.
  • We need better ways to include external themes

One fundamental aspect for addressing the first two points is theme versioning: We need something like

plt.style.use("dark_background-v1")  # The current style
plt.style.use("dark_background-v2")  # The new style
plt.style.use("dark_background")  # Use the newest version available in your matplotlib installation.

People who need stability can pin a version. People who want follow updates use the unpinned version.

This would also help e.g. with the seaborn style syncing issue #22317, #13680, #14331.

jklymak commented Aug 11, 2022

Copy link
Copy Markdown
Member

We should discuss on the call, but I'm not convinced having a "pinnable" style, but the main style changing at arbitrary points in the dev cycle is very desirable either. If you have old code, and you understandably didn't realize you needed to pin, its still going to cause reproducibility problems.

I'm also concerned with PR churn as folks tweak styles. "surely no one likes the ticks [that long/pointing outward/that thick]", "publishable plots must use [latex fonts/serifed fonts/sans-serif]", "surely no one wants the title [so large, so small]". etc etc. and then we change it, and the original designer will rightly ask "why did you mess with the style I made"?

The current method to inject a new style is to put the style sheet somewhere and reference it, or put it in mpl_configdir/stylib where we will automatically pick it up. https://matplotlib.org/stable/tutorials/introductory/customizing.html#defining-your-own-style. That seems pretty flexible and straight forward to me?

timhoffm commented Aug 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Unfortunately, I'll not be on the call today.

If you have old code, and you understandably didn't realize you needed to pin, its still going to cause reproducibility problems

This could only be prevented by a "we never change a style ever" policy, which IMHO is overly restictive and has downsides of its own.
OTOH pinning can at least be applied later to fix a reproducibility problem.

I'm also concerned with PR churn as folks tweak styles.

We'd still have a policy what and how often we change. That should be reasonably conservative.

The current method to inject a new style is to put the style sheet somewhere and reference it

First and foremost, versioning will allow improving the existing styles. Whether we want to add new styles is a separate concern.

jklymak commented Aug 11, 2022

Copy link
Copy Markdown
Member

This could only be prevented by a "we never change a style ever" policy,

In recent times, Matplotlib only changed styles in the 2.0-3.0 transition, with lots of notice and advertisement.

story645 commented Aug 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

So as discussed on the call, closing this for now cause we have to sort out the policy and we'll move discussion of styles to the appropriate issues.

Thanks so much @ubitux for the very quick turnaround on the PR and spurring the discussion & sorry for any time wasted on your part during this back and forth.

ETA If you can make it, please join our dev or new contributor calls to discuss style changes/process for changes & anything else about contributing at the new contributor call. https://scientific-python.org/calendars/

story645 closed this Aug 11, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: default changes Changes to default behavior status: needs comment/discussion needs consensus on next step topic: styles

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL