Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mplot3d] Add custom values for determining colors in trisurf #23878

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aaarne
Copy link

@aaarne aaarne commented Sep 12, 2022

PR Summary

This PR adds the possibility to color faces individually in plot_trisurf. Before one could only use one color for all faces, or use a colormap encoding the mean z-coordinate of the vertices at each triangle for the triangle's surface. In this PR the function get a new keyword argument called c, defaulting to None. One can now specify a value for each vertex via c. Then these values are used to determine face-colors via the colormap, not the mere z-coordinates.

Example

import matplotlib.tri as mtri
import matplotlib.pyplot as plt

fig = plt.figure()
ax = fig.add_subplot(projection='3d')

r = np.linspace(0, np.pi, 50)
phi = np.linspace(-np.pi, np.pi, 50)
r, phi = np.meshgrid(r, phi)
r, phi = r.flatten(), phi.flatten()
tri = mtri.Triangulation(r, phi)

x = np.sin(r)*np.cos(phi)
y = np.sin(r)*np.sin(phi)
z = np.cos(r)

ax.plot_trisurf(x, y, z, triangles=tri.triangles, c=phi, cmap=plt.cm.Spectral)

image

Plots like this were not possible, even not with the color keyword argument - this only allowed for a single color.

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).
  • [N/A] 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).

Copy link

@github-actions github-actions bot 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 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.

@oscargus
Copy link
Contributor

Thanks! Can you please add a test for this? I think that maybe your example could be a good one.

@ianthomas23
Copy link
Member

Note that this is adding a 3D version of tripcolor functionality. That does at need to be specified in the docs somewhere so that users of 2D tripcolor wanting to move to 3D can search for it and end up here. Also, in the 2D tripcolor the argument is called C (the array of color values) which is probably a better choice than values here on the grounds of consistency.

@QuLogic
Copy link
Member

QuLogic commented Sep 12, 2022

[N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).

This appears to be a new feature; I'm not sure why you marked it as N/A.

@aaarne
Copy link
Author

aaarne commented Sep 13, 2022

Note that this is adding a 3D version of tripcolor functionality. That does at need to be specified in the docs somewhere so that users of 2D tripcolor wanting to move to 3D can search for it and end up here. Also, in the 2D tripcolor the argument is called C (the array of color values) which is probably a better choice than values here on the grounds of consistency.

It is not really the same behavior as in tripcolor. In the 2D tripcolor the C allows to define colors directly for each vertex or face. Here in my PR, the specified values are first color-mapped and then the faces are colored with the result.

  • I agree that a C argument specifying individual face colors manually is a good idea and is a missing feature in the implementation.
  • The values allow specifying values at each vertex, which are then used to determine colors at each face via the colormap. This is different, but I believe it is still a very convenient and needed feature. Often we know point-data on the vertices and wanna colorize it accordingly. We just need to decide a name for the keyword argument. I agree that values is not a good name. Suggestions?

@aaarne
Copy link
Author

aaarne commented Sep 13, 2022

Note that this is adding a 3D version of tripcolor functionality. That does at need to be specified in the docs somewhere so that users of 2D tripcolor wanting to move to 3D can search for it and end up here. Also, in the 2D tripcolor the argument is called C (the array of color values) which is probably a better choice than values here on the grounds of consistency.

It is not really the same behavior as in tripcolor. In the 2D tripcolor the C allows to define colors directly for each vertex or face. Here in my PR, the specified values are first color-mapped and then the faces are colored with the result.

* I agree that a `C` argument specifying individual face colors manually is a good idea and is a missing feature in the implementation.

* The `values` allow specifying values at each vertex, which are then used to determine colors at each face via the colormap. This is different, but I believe it is still a very convenient and needed feature. Often we know point-data on the vertices and wanna colorize it accordingly. We just need to decide a name for the `keyword` argument. I agree that `values` is not a good name. Suggestions?

Should it use C for both options? When C is shape (n,3/4) use it as colors directly and if shape is (n,) map it first? Actually scatter does it this way, there it's a small c, though.

@aaarne
Copy link
Author

aaarne commented Sep 13, 2022

[N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).

This appears to be a new feature; I'm not sure why you marked it as N/A.

I though the change is so small, that it cannot really be called a new feature, but okay, will add it to whats new.

@aaarne
Copy link
Author

aaarne commented Sep 13, 2022

Thank you all for the input. I did some changes to the PR:

  • the new keyword argument for custom data used for color-coding is now called c, as in plt.scatter(..). It is on purpose not called C as in tripcolor, because there we give colors directly, not values to be mapped to colors. The data in c can be one value per vertex OR one value per face. Which of these options applies is determined based on the shape - as in tripcolor. If no colormap is specified, then c is interpreted as a row-wise rgb/rgba color matrix, again either in terms of vertex colors (averaged on face) or face colors. This then resembles the behavior of C in tripcolor.
  • I have written two test cases
  • There is now a What's New article
  • There is also an example file
  • As in triplot there is also a new keyword argument called facecolors, which can be used to directly set the face colors as RGB or RGBA values.

@ianthomas23
Copy link
Member

It is on purpose not called C as in tripcolor, because there we give colors directly, not values to be mapped to colors.

Look again!

import matplotlib.pyplot as plt

x = [0, 1, 1, 0]
y = [0, 0, 1, 1]
C = [0, 1, 2, 3]
fig, ax = plt.subplots()
mappable = ax.tripcolor(x, y, C)
fig.colorbar(mappable, ax=ax)
plt.show()

Figure_1

@aaarne
Copy link
Author

aaarne commented Sep 14, 2022

Oh, okay. You are right. I have not tried it, just read the documentation here https://matplotlib.org/3.1.1/api/_as_gen/matplotlib.pyplot.tripcolor.html

There it says that C must be color values or I got the text wrong. It is a bit misleading there in the documentation. I will update the documentation of tripcolor and rename my c to C okay?

@oscargus
Copy link
Contributor

This is the warning in the failing doc-build:

/home/circleci/project/doc/gallery/mplot3d/trisurf3d_3.rst:24: WARNING: Title overline too short.

I think that you may need to create a new branch as there are changes on the main branch that are required to make the tests pass and since you have used your main branch, it is not really possible (in an easier way) to sort it out. Something like:

git branch trisurfcustomcolor
git reset --hard HEAD~7
git pull upstream main
git checkout trisurfcustomcolor
git rebase main
git push origin trisurfcustomcolor

And then open a new PR with the trisurfcustomcolor branch.

You may have to set upstream if it complains in the third step:

git remote add upstream git@github.com:matplotlib/matplotlib.git

Here is a more general description: https://stackoverflow.com/questions/1628563/move-the-most-recent-commits-to-a-new-branch-with-git

@timhoffm
Copy link
Member

I think the casing c vs C is not consistent in our docs. In general, we tend to use uppercase for 2D arrays (think X, Y = np.meshgrid(x, x)), e.g. pcolormesh. I suspect that the tripcolor uppercase C leaked from there.

@timhoffm
Copy link
Member

@aaarne Do you plan to come back to this? Let us know in case you need any help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

None yet

5 participants