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

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

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

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

Alright, rebased.

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

flake8 failure

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)

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

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

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

@WeatherGod:

cross ref to the LIghtSource class.

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

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

@timhoffm: Done

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.

*cmap*

We use *cmap* to denote other variables.

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

*shade*

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

period

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.

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`.

We use *shade* to denote other variables.

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

Looks like I missed this instance, sorry.

…der correctly

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

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.

👍 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

+                          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

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

+                          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.

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

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

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

Repository navigation

Fix and document lightsource argument in mplot3d - #9990

Fix and document lightsource argument in mplot3d#9990dstansby merged 5 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom eric-wieser:lightsource-shadingeric-wieser/matplotlib:lightsource-shadingCopy head branch name to clipboard

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix and document lightsource argument in mplot3d#9990
dstansby merged 5 commits into
matplotlib:masterfrom
eric-wieser:lightsource-shading

eric-wieser commented Feb 9, 2018 •
edited
Loading

timhoffm Aug 21, 2018 •
edited
Loading

timhoffm commented Sep 6, 2018 •
edited
Loading