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.

Sign up for GitHub

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

Jump to bottom

Make markerfacecolor work for 3d scatterplots #10693

Closed

jinshifen33 wants to merge 4 commits into matplotlib:master from jinshifen33:bugfix-for-issue-9725

Contributor

jinshifen33 commented Mar 5, 2018 •

edited

Loading

PR Summary

This pull request is a fix for issue 9725 color issue on 3D scatter plots. Invoking the set color method after the initial creation of a scatter 3D plot would not change the color; these changes fix this issue.

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jinshifen33 added 3 commits

March 5, 2018 13:44


          fix for 9725

0d4b3c7


          Add:

27d9699

1. PEP8 compliant
2. 1 more test case
3. doc for new methods


          follow up with master

723d617

Contributor Author

jinshifen33 commented Mar 5, 2018

One thing I noticed is that, the edge_color will never equal to "face" in 3D, this fix also introduces a workaround for it, I am not sure whether this is desired behaviour .

timhoffm requested changes

View reviewed changes

lib/mpl_toolkits/mplot3d/art3d.py Outdated

@@ @@ -449,24 +450,62 @@ def set_3d_properties(self, zs, zdir): @@
                       self._edgecolor3d = self.get_edgecolor()
                       self.stale = True
+                  def set_facecolor(self, colors):
+                      # override setter for facecolor so that 3dfacecolor

Member

timhoffm Mar 5, 2018

Please use triple-quote docstrings instead of comments for documenting functions. This is the standard pep8 style we're aiming for (even if historically there are other styles in the code base).

lib/mpl_toolkits/mplot3d/art3d.py Outdated

+                      # In providing consitentcy in 2dcolor and 3dcolor setter.
+                      # Note: will also update edgecolor if it follows facecolor
+                      PathCollection.set_facecolor(self, colors)

Member

timhoffm Mar 5, 2018

Since this will by py3 only, use super().

lib/mpl_toolkits/mplot3d/art3d.py Outdated

+                      self._edgecolor3d = PathCollection.get_edgecolor(self)
+                  set_edgecolors = set_edgecolor
+                  def _set_facecolors_on_project(self, colors):

Member

timhoffm Mar 5, 2018

what is project? Should this rather be _set_facecolors_on_projection or _set_facecolors_on_2d_projection?

lib/mpl_toolkits/mplot3d/art3d.py Outdated

+                  set_edgecolors = set_edgecolor
+                  def _set_facecolors_on_project(self, colors):
+                      # A sepearted method that changes edgecolor in drawing

Member

timhoffm Mar 5, 2018

facecolor

lib/mpl_toolkits/mplot3d/art3d.py Outdated

                   def do_3d_projection(self, renderer):
                       xs, ys, zs = self._offsets3d
                       vxs, vys, vzs, vis = proj3d.proj_transform_clip(xs, ys, zs, renderer.M)
                       fcs = (zalpha(self._facecolor3d, vzs) if self._depthshade else
                              self._facecolor3d)
                       fcs = mcolors.to_rgba_array(fcs, self._alpha)
-                      self.set_facecolors(fcs)
+                      # now uses setter on project

Member

timhoffm Mar 5, 2018

The comment does not add information and thus can be left out.


          Fix naming convention and docstring

22c2d62

jklymak added this to the v3.0 milestone

jklymak changed the title ~~Bugfix for issue 9725~~ Make markerfacecolor work for 3d scatterplots

Member

jklymak commented Mar 5, 2018

Milestoning 3.0 because this isn't fixing a regression.

Member

jklymak commented Mar 6, 2018

Thanks a lot for your contribution.

However, this conflicts with an existing PR: #10489 I'm going to close this PR to reduce confusion. If you want to help, you can make comments on #10489. (Also if you want to see if someone else has made a PR based on an issue, its usually cross-linked at the bottom of the issue.)

jklymak closed this

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