Fix edge color, links, wording; closes matplotlib/matplotlib#23895 by baharev · Pull Request #23930 · matplotlib/matplotlib · GitHub

PR Summary

See #23895:

the 3D surface was unrecognizable due to the invisible edges,
the links in the documentation were broken,
the wording in the title did not conform to the guidelines.

Closes #23895.

PR Checklist

Tests and Styling

[N/A] Has pytest style unit tests (and pytest passes).
[N/A] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

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

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.

https://output.circle-artifacts.com/output/job/d7499f04-4c17-4825-9f71-2fc176c546ea/artifacts/0/doc/build/html/gallery/mplot3d/contourf3d_2.html#sphx-glr-gallery-mplot3d-contourf3d-2-py

https://output.circle-artifacts.com/output/job/d7499f04-4c17-4825-9f71-2fc176c546ea/artifacts/0/doc/build/html/gallery/mplot3d/contour3d_3.html#sphx-glr-gallery-mplot3d-contour3d-3-py

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.

I think it would look a bit better with a thinner lw on the surface?

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, but https://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 Poly3DCollection uses patch.linewidth: 1.0 by default.

For comparison grid.linewidth: 0.8. So yes, the lines are thicker. I suppose the lines appear even more prominent because of the black color.

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 plot_surface call. I'm therefore not worried adding more to make the plot look nicer.

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 lw=0.5

edgecolor='black'

edgecolor='cm.coolwarm(0)'

edgecolor='0.35'

I find the one in the middle the nicest, but I have serious trouble with colors. So, please vote.

I find the one in the middle the nicest.

I agree. There's some consistency in having darker blue lines on a light-blue surface. Could you also try tab:blue as a color? I think the faces are tab:blue but light because of the transparency.

Thank you for your prompt feedback. Here is the one with tab:blue:

I find the one with edgecolor='cm.coolwarm(0)' a bit more vivid (nicer).

Thanks!

I find the one with edgecolor='cm.coolwarm(0)' a bit more vivid (nicer).

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.

It's recommended nowadays to use the colormap registry instead of importing from matplotlib.cm:

cmap = plt.colormaps['coolwarm']

You could go even one step further and use the string contour(..., cmap='coolwarm'). Usually, that's the simplest way if you don't need the colormap object itself. Here however, you have the cmap(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 retrieve cmap from plt.colormaps.

Yes, that cmap(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!

You can try 'mediumblue' which is slightly darker, or 'royalblue' which is a bit lighter.

As above.

I do not know how to link the built

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:

https://output.circle-artifacts.com/output/job/50494046-2293-4d95-b535-e05809d0f98a/artifacts/0/doc/build/html/gallery/mplot3d/contour3d_3.html#sphx-glr-gallery-mplot3d-contour3d-3-py

https://output.circle-artifacts.com/output/job/50494046-2293-4d95-b535-e05809d0f98a/artifacts/0/doc/build/html/gallery/mplot3d/contourf3d_2.html

I went with royalblue. In my opinion, the mediumblue looks too harsh on the filled version of the plot.

What should I do next?

…atplotlib#23895

Thank you @baharev and congratulations on your first merged Matplotlib PR 🎉

I hope that we will hear from you again!

You maybe interested in
https://www.youtube.com/watch?v=Gapv8wR5DYU&list=PLYx7XA2nY5Ge3LsWy500pi5bdHEiAdQB5&index=4
Development of Accessible, Aesthetically-Pleasing Color Sequences - Matthew Petroff (@mpetroff) from Scipy 2022.

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.

…930-on-v3.6.x Backport PR #23930 on branch v3.6.x (Fix edge color, links, wording; closes #23895)

Fix edge color, links, wording; closes #23895

5f67a3d

github-actions Bot reviewed Sep 18, 2022

View reviewed changes

baharev mentioned this pull request Sep 18, 2022

[Bug]: 3D surface is not plotted for the contour3d_3 example in the gallery #23895

Closed

tacaswell added this to the v3.6.1 milestone Sep 18, 2022

tacaswell approved these changes Sep 18, 2022

View reviewed changes

Changing the edge color and the line width in response to the comments

85e6634

timhoffm approved these changes Sep 19, 2022

View reviewed changes

timhoffm reviewed Sep 19, 2022

View reviewed changes

Specifying the color map and the edge color as strings

ff7c43a

tacaswell merged commit f44e70e into matplotlib:main Sep 20, 2022

meeseeksmachine mentioned this pull request Sep 20, 2022

Backport PR #23930 on branch v3.6.x (Fix edge color, links, wording; closes matplotlib/matplotlib#23895) #23966

Merged

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 20, 2022

Backport PR matplotlib#23930 : Fix edge color, links, wording; closes m…

899983c

…atplotlib#23895

oscargus added a commit that referenced this pull request Sep 21, 2022

Merge pull request #23966 from meeseeksmachine/auto-backport-of-pr-23…

5bef1a2

…930-on-v3.6.x Backport PR #23930 on branch v3.6.x (Fix edge color, links, wording; closes #23895)

ksunden mentioned this pull request Feb 20, 2023

merge up 3.7.0 #25270

Merged

6 tasks

               from mpl_toolkits.mplot3d import axes3d
               import matplotlib.pyplot as plt
-              from matplotlib import cm
+              from matplotlib.cm import coolwarm as cmap

               from mpl_toolkits.mplot3d import axes3d
               import matplotlib.pyplot as plt
-              from matplotlib import cm
+              from matplotlib.cm import coolwarm as cmap

Fix edge color, links, wording; closes matplotlib/matplotlib#23895 by baharev · Pull Request #23930 · matplotlib/matplotlib · GitHub

Repository navigation

Fix edge color, links, wording; closes matplotlib/matplotlib#23895 - #23930

Fix edge color, links, wording; closes matplotlib/matplotlib#23895#23930tacaswell merged 3 commits intomatplotlib:mainmatplotlib/matplotlib:mainfrom baharev:mainCopy head branch name to clipboard

Conversation

baharev commented Sep 18, 2022

PR Summary

PR Checklist

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Sep 18, 2022

Uh oh!

tacaswell commented Sep 18, 2022

Uh oh!

baharev commented Sep 18, 2022

Uh oh!

tacaswell commented Sep 18, 2022

Uh oh!

timhoffm commented Sep 18, 2022

Uh oh!

oscargus commented Sep 19, 2022

Uh oh!

timhoffm commented Sep 19, 2022

Uh oh!

baharev commented Sep 19, 2022

Uh oh!

timhoffm commented Sep 19, 2022

Uh oh!

baharev commented Sep 19, 2022

Uh oh!

timhoffm commented Sep 19, 2022

Uh oh!

baharev commented Sep 19, 2022

Uh oh!

timhoffm Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

baharev Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffm Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffm Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffm commented Sep 19, 2022

Uh oh!

baharev commented Sep 20, 2022

Uh oh!

tacaswell commented Sep 20, 2022

Uh oh!

baharev commented Sep 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix edge color, links, wording; closes matplotlib/matplotlib#23895#23930
tacaswell merged 3 commits into
matplotlib:mainfrom
baharev:main