Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix edge color, links, wording; closes matplotlib/matplotlib#23895#23930
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this 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.
Are the built updated pages. I am 👍🏻 to merge as is, but I think it would look a bit better with a thinner lw on the surface? |
Thank you for finding and fixing this very old fallout from the mpl 1->2 style change! |
@tacaswell Thanks for the feedback.
Agreed, those plots are ugly. However, this is also very strange: It is not how it looks like on my machine locally. How should I fix this? |
Do you have any custom rcparams set? The simplest solution is to pass lw to the calls, but we should understand why you get something different locally. |
I did not check whether it's related, buthttps://matplotlib.org/devdocs/gallery/mplot3d/wire3d.html#sphx-glr-gallery-mplot3d-wire3d-py has a similar linewidth. |
It would indeed look better with thinner lines, but then this seems to be the default linewidth considering the width of the other plotted lines in that figure (the ones on the panes)? |
The For comparison I suggest to make the lines thinner (0.8 or even 0.5) and maybe go from black to a dark grey. We already a lot of customization on the |
I am someone with serious color vision deficiency, so I am kindly asking you to vote. Which one of these looks the best? Each with
I find the one in the middle the nicest, but I have serious trouble with colors. So, please vote. |
I agree. There's some consistency in having darker blue lines on a light-blue surface. Could you also try |
Thank you for your prompt feedback. Here is the one with
I find the one with |
Thanks!
Agreed. Let's use that one. |
Done. All green. I do not know how to link the built updated pages here, sorry. They look really nice locally on my machine. |
examples/mplot3d/contour3d_3.py Outdated
| """ | ||
| from mpl_toolkits.mplot3d import axes3d | ||
| import matplotlib.pyplot as plt | ||
| from matplotlib importcm | ||
| from matplotlib.cm importcoolwarm as cmap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's recommended nowadays to use the colormap registry instead of importing frommatplotlib.cm:
cmap = plt.colormaps['coolwarm']You could go even one step further and use the stringcontour(..., cmap='coolwarm'). Usually, that's the simplest way if you don't need the colormap object itself. Here however, you have thecmap(0) call. I'm tempted to resolve this to an explicit'#3b4cc0' and get rid of the colormap object completely, which IMHO makes the example simpler.
But I'll let you choose whether to go all-string or retrievecmap fromplt.colormaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, thatcmap(0) is bugging me too. I was considering'#3b4cc0' too, but it is too cryptic. Is there some named blue in the neighborhood?
https://matplotlib.org/stable/gallery/color/named_colors.html#css-colors
Since I am green blind, I would like to kindly ask you to please help me out here: If you agree, please suggest a named blue instead of'#3b4cc0'. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can try 'mediumblue' which is slightly darker, or 'royalblue' which is a bit lighter.
examples/mplot3d/contourf3d_2.py Outdated
| """ | ||
| from mpl_toolkits.mplot3d import axes3d | ||
| import matplotlib.pyplot as plt | ||
| from matplotlib importcm | ||
| from matplotlib.cm importcoolwarm as cmap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As above.
FYI: There's a "View the built docs" entry in the CI checks section below:
|
Thank you very much for your help. The revised code/plots are here: I went with royalblue. In my opinion, the mediumblue looks too harsh on the filled version of the plot. What should I do next? |
Thank you@baharev and congratulations on your first merged Matplotlib PR 🎉 I hope that we will hear from you again! You maybe interested in One of my regrets from the 2.0 default style changes is that while we put a whole lot of effort into the color map and making sure it was OK for color vision deficiencies, we failed to do the same to the default color cycle. |
And I would like to thank you all for your help, patience and constructive feedback. 🙂 I will watch that video this evening, thanks for the info. |
PR Summary
See#23895:
Closes#23895.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).