Fix and document lightsource argument in mplot3d by eric-wieser · Pull Request #9990 · matplotlib/matplotlib

eric-wieser · 2017-12-13T06:36:15Z

PR Summary

Fixes the problems in #8877, where the lightsource argument is broken in silly ways.

This has been sitting in my working tree a while now, but is freshly rebased.

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

eric-wieser · 2017-12-13T07:42:53Z

This will conflict with #9991, so lets deal with #9991 first

eric-wieser · 2018-02-09T03:58:18Z

#9991 is in, but I think this will conflict with #10001 too - so again, I plan to delay this one

eric-wieser · 2018-07-12T06:59:42Z

Alright, rebased.

WeatherGod · 2018-07-12T16:19:03Z

./lib/mpl_toolkits/mplot3d/axes3d.py:1652:9: E303 too many blank lines (2)
1     E303 too many blank lines (2)

flake8 failure

timhoffm

Minor style issue: Always put a period at the end of a Parameter description, even if it‘s just an Expression like „Data values as 1D arrays.“. (Multiple occurrences)

WeatherGod · 2018-07-13T13:37:39Z

lib/mpl_toolkits/mplot3d/axes3d.py

+            always disabled when cmap is specified
+
+        lightsource : LightSource
+            The lightsource to use when `shade` is True


would be nice if this referenced the LightSource api docs since this isn't a common thing.

WeatherGod · 2018-07-13T13:45:14Z

lib/mpl_toolkits/mplot3d/axes3d.py

+            Whether to shade the facecolors. Defaults to True. Shading is
+            always disabled when cmap is specified
+        lightsource : LightSource
+            The lightsource to use when `shade` is True


cross ref to the LIghtSource class. Also, would it make sense to give a rough description of what the default light source would be?

This didn't respect the x and y coordinates, so plotting `x{::-1], y{::-1], z[::-1,::-1]` would result in different shading to `x, y, z`. It also looked super screwy, and could only be triggered if `facecolors="do something wacky"` was passed

eric-wieser · 2018-07-14T06:22:52Z

PEP8 and periods fixed. Feel free to edit docs in place here

eric-wieser · 2018-08-10T04:57:21Z

@WeatherGod:

cross ref to the LIghtSource class.

Can I do that directly in the parameter types as lightsource : `LightSource` ?

timhoffm · 2018-08-13T16:55:22Z

Please use: lightsource : `~matplotlib.colors.LightSource`

eric-wieser · 2018-08-19T18:35:59Z

@timhoffm: Done

timhoffm

Some docstring issues to be fixed (sorry to be a bit picky here, but we want to have good looking consistent documentation).

Otherwise looks good.

timhoffm · 2018-08-21T21:11:07Z

lib/mpl_toolkits/mplot3d/axes3d.py

+            Minimum and maximum value to map.
+        shade : bool
+            Whether to shade the facecolors. Defaults to True. Shading is
+            always disabled when `cmap` is specified.


*cmap*

We use *cmap* to denote other variables.

https://matplotlib.org/devel/documenting_mpl.html#id3

timhoffm · 2018-08-21T21:11:24Z

lib/mpl_toolkits/mplot3d/axes3d.py

+            Whether to shade the facecolors. Defaults to True. Shading is
+            always disabled when `cmap` is specified.
+        lightsource : `~matplotlib.colors.LightSource`
+            The lightsource to use when `shade` is True.


timhoffm · 2018-08-21T21:12:45Z

lib/mpl_toolkits/mplot3d/axes3d.py

+        Parameters
+        ----------
+        X, Y, Z : array-like
+            Data values as 1D arrays


Descriptions should end with a period, even if they are only expressions and not full sentences.

timhoffm · 2018-08-21T21:12:55Z

lib/mpl_toolkits/mplot3d/axes3d.py

+        X, Y, Z : array-like
+            Data values as 1D arrays
+        color
+            Color of the surface patches


timhoffm · 2018-08-21T21:19:12Z

lib/mpl_toolkits/mplot3d/axes3d.py

+        lightsource : `~matplotlib.colors.LightSource`
+            The lightsource to use when `shade` is True.

        The (optional) triangulation can be specified in one of two ways;


There can be no text block in a parameters section in numpydoc style. This would be interpreted as further parameters, which doesn't look nice

Please move this block before the parameters section.

timhoffm · 2018-08-21T21:24:17Z

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -1917,7 +1922,8 @@ def plot_trisurf(self, *args, color=None, norm=None, vmin=None, vmax=None,
        Other arguments are passed on to


Same text block in parameters section problem. We work around that by summarizing this under a **kwargs parameter:

**kwargs All other keyword argument are passed to `~mpl_toolkits.mplot3d.art3d.Poly3DCollection`.

timhoffm · 2018-08-21T21:25:40Z

lib/mpl_toolkits/mplot3d/axes3d.py

+            always disabled when `cmap` is specified.
+
+        lightsource : `~matplotlib.colors.LightSource`
+            The lightsource to use when `shade` is True.


We use *shade* to denote other variables.

https://matplotlib.org/devel/documenting_mpl.html#id3

Looks like I missed this instance, sorry.

…der correctly

eric-wieser · 2018-09-06T05:39:03Z

@timhoffm: Updated. I'm starting to regret updating the documentation to the new format...

timhoffm · 2018-09-06T06:07:58Z

I'm starting to regret updating the documentation to the new format...

I'm sorry about that. I know that changing the documentation format is quite a hassle. However, a good and consistent documentation is essential for a complex library like matplotlib. We still have a long way to go, but we have already made some progress with the docs. It's a long-running effort which has to be taken one small step at a time. So thanks again for helping with this.

This is good to go. We just need a second review.

dstansby

👍 Thanks a lot! This looks like something that could do with a figure test at some point, but I'll leave that until another PR.

eric-wieser force-pushed the lightsource-shading branch from bf013b7 to 915e522 Compare December 13, 2017 06:39

eric-wieser changed the title ~~Lightsource shading~~ Fix and document lightsource argument in mplot3d Dec 13, 2017

tacaswell added this to the v2.2 milestone Dec 13, 2017

tacaswell added the topic: mplot3d label Dec 13, 2017

tacaswell requested a review from WeatherGod December 13, 2017 19:59

eric-wieser mentioned this pull request May 23, 2018

MAINT/BUG: Don't use 5-sided quadrilaterals in Axes3D.plot_surface #10001

Merged

BUG: Respect the lightsource argument in plot_surface and plot_trisurf

833543d

eric-wieser force-pushed the lightsource-shading branch from 915e522 to 57fe938 Compare July 12, 2018 06:57

timhoffm reviewed Jul 13, 2018

View reviewed changes

WeatherGod reviewed Jul 13, 2018

View reviewed changes

eric-wieser added 2 commits July 13, 2018 16:26

DOC: Add documentation for lightsource parameter

9e73e25

BUG: remove broken shading in plot_surface

ec18ea9

This didn't respect the x and y coordinates, so plotting `x{::-1], y{::-1], z[::-1,::-1]` would result in different shading to `x, y, z`. It also looked super screwy, and could only be triggered if `facecolors="do something wacky"` was passed

eric-wieser force-pushed the lightsource-shading branch from 57fe938 to ec18ea9 Compare July 13, 2018 16:57

DOC: Add sphinx links to the LightSource class

4749752

timhoffm approved these changes Aug 21, 2018

View reviewed changes

DOC: Use emphasis for other parameters, fix parameters section to ren…

b056107

…der correctly

timhoffm modified the milestones: needs sorting, v3.1 Sep 6, 2018

timhoffm added the status: needs review label Sep 6, 2018

dstansby approved these changes Sep 16, 2018

View reviewed changes

dstansby merged commit 82a4b24 into matplotlib:master Sep 16, 2018

eric-wieser mentioned this pull request Sep 16, 2018

MAINT: Unify calculation of normal vectors from polygons #12136

Merged

6 tasks

anntzer mentioned this pull request Sep 17, 2018

Unbreak build re: mplot3d style. #12139

Merged

6 tasks

QuLogic removed the status: needs review label Sep 23, 2018

eric-wieser deleted the lightsource-shading branch March 23, 2019 18:07

		@@ -1917,7 +1922,8 @@ def plot_trisurf(self, *args, color=None, norm=None, vmin=None, vmax=None,
		Other arguments are passed on to

Uh oh!

Conversation

eric-wieser commented Dec 13, 2017

PR Summary

PR Checklist

Uh oh!

eric-wieser commented Dec 13, 2017

Uh oh!

eric-wieser commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Jul 12, 2018

Uh oh!

WeatherGod commented Jul 12, 2018

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Jul 14, 2018

Uh oh!

eric-wieser commented Aug 10, 2018

Uh oh!

timhoffm commented Aug 13, 2018

Uh oh!

eric-wieser commented Aug 19, 2018

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Sep 6, 2018

Uh oh!

timhoffm commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eric-wieser commented Feb 9, 2018 •

edited

Loading

timhoffm Aug 21, 2018 •

edited

Loading

timhoffm commented Sep 6, 2018 •

edited

Loading