Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

style: make grid more discreet in dark_background#23598

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
ubitux wants to merge1 commit intomatplotlib:mainfromubitux:nicer-dark-grid
Closed

style: make grid more discreet in dark_background#23598

ubitux wants to merge1 commit intomatplotlib:mainfromubitux:nicer-dark-grid

Conversation

ubitux
Copy link

PR Summary

Currently:
grid-cur

Proposed:
grid-fix

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

@story645
Copy link
Member

story645 commentedAug 10, 2022
edited
Loading

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.

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

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 uson gitter for real-time discussion.

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

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

@ubitux
Copy link
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 like777777 maybe?

@story645
Copy link
Member

story645 commentedAug 10, 2022
edited
Loading

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

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

@ubitux
Copy link
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 withbbbbbb (73%):

bbbbbb

And here is777777:

777777

story645 reacted with thumbs up emoji

@jklymak
Copy link
Member

jklymak commentedAug 10, 2022
edited
Loading

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

@ubitux
Copy link
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 thedark_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 to777777?

Also, is the CI supposed to pass currently?

@jklymak
Copy link
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.

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

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

We can't just change an existing style.

@ubitux
Copy link
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

@ubituxubitux closed thisAug 10, 2022
@story645
Copy link
Member

story645 commentedAug 10, 2022
edited
Loading

@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

@ubituxubitux reopened thisAug 10, 2022
@ubitux
Copy link
Author

ubitux commentedAug 10, 2022
edited
Loading

Here are the default and different flavors:

colors

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

@story645
Copy link
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

@QuLogicQuLogic added the status: needs comment/discussionneeds consensus on next step labelAug 10, 2022
@tacaswell
Copy link
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.

Copy link
Member

@tacaswelltacaswell left a comment

Choose a reason for hiding this comment

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

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

@story645
Copy link
Member

story645 commentedAug 10, 2022
edited
Loading

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 thematplotlib docs:
image
I'm honestly unsure here if the grid is a different color or appears as such because it is so much thinner than the axishttps://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
Copy link
Member

timhoffm commentedAug 11, 2022
edited
Loading

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 styleplt.style.use("dark_background-v2")  # The new styleplt.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.

story645 and mwaskom reacted with thumbs up emojistory645 reacted with heart emoji

@jklymak
Copy link
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 inmpl_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
Copy link
Member

timhoffm commentedAug 11, 2022
edited
Loading

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
Copy link
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
Copy link
Member

story645 commentedAug 11, 2022
edited
Loading

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/

noatamir reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@tacaswelltacaswelltacaswell requested changes

@jklymakjklymakjklymak requested changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@ubitux@story645@jklymak@tacaswell@timhoffm@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp