Skip to content

bug fix - set_facecolors not working for 3D scatter #10489

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

Closed

Conversation

xiaoshiqi
Copy link

PR Summary

fixed set_facecolor(set_facecolors), set_edgecolor(set_edgecolors), and set_color not working for scatter patch in 3D #9725
_facecolor3d, _edgecolor3d should be updated alone with color changes.

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

@phobson
Copy link
Member

phobson commented Feb 16, 2018

@xiaoshiqi thanks for the patch! I think this will be a very useful addition. Could you add a test that confirms that this works as expected?

I'm not sure if you need to make a full-on image comparison test for this, but a test should at least create a 3D artist with a certain face and edge color, set them to something different, and the confirm that the changes stick as a expected.

The mplot3d tests are here:
https://github.com/matplotlib/matplotlib/blob/master/lib/mpl_toolkits/tests/test_mplot3d.py

@xiaoshiqi
Copy link
Author

no problem, I will work on it asap

rollback docstring to original
add a test case
@WeatherGod
Copy link
Member

I am not quite understanding the code design here. Why wasn't this added to the Patch3DCollection class directly?

@xiaoshiqi
Copy link
Author

@WeatherGod I'm afraid that other artists may be affected if I modify Patch3DCollection.

Copy link
Member

@WeatherGod WeatherGod left a comment

Choose a reason for hiding this comment

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

I am afraid I'll have to reject these changes if they are applied only for artists that come from calling scatter3d(). They should be applied to the appropriate artist in art3d.py

@jklymak jklymak changed the title bug fix - set_facecolors not working for 3D scatter (#9725) bug fix - set_facecolors not working for 3D scatter Mar 6, 2018
@xiaoshiqi
Copy link
Author

@WeatherGod I have attempted another fix. Could you please review this?
I have not yet push the test cases, currently I am having a little issue with image comparison RMS != 0
Will push them soon once I fix the image comparison tests.

@xiaoshiqi
Copy link
Author

@WeatherGod it's now fixed

@xiaoshiqi xiaoshiqi closed this Mar 15, 2018
@WeatherGod
Copy link
Member

why is this closed? did another PR fix the problem? Sorry I have been quiet lately, multiple snowstorms and sicknesses have eaten up my free time.

@xiaoshiqi
Copy link
Author

@WeatherGod I'm sorry, I thought this PR is rejected, so I started another one #10797

@WeatherGod
Copy link
Member

WeatherGod commented Mar 16, 2018 via email

@xiaoshiqi
Copy link
Author

@WeatherGod thanks

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

Successfully merging this pull request may close these issues.

4 participants