DOC Clean up on about half the Mplot3d examples #6303

Merged
merged 7 commits into from Apr 19, 2016

Conversation

Projects
None yet
4 participants
Contributor

TrishGillett commented Apr 14, 2016

My goal is to make it easy to understand what the code is doing, so people flipping through examples can quickly judge if an example has something they can use, and so people borrowing a bit of code will understand how to modify it responsibly, so to speak.

There are guidelines on MEP12 and comments on issue #6221 about what examples should strive to do. I may not have ticked every single box mentioned in those places, but I've made the formatting a lot more consistent, added a lot of docstrings and comments, and made other mostly cosmetic changes. Let me know if there are any particular style points you'd like to see done differently.

I made one nontrivial change, to the behaviour of polys3d_demo. I changed it so that it forms the bottom facet of each polygon by creating two new vertices rather than overwriting two of the y data points. I thought it was better to assume that a user might want to do this using some data of theirs which shouldn't be overwritten.

mdboom changed the title from DOC Clean up on about half the Mplot3d examples to DOC Clean up on about half the Mplot3d examples Apr 14, 2016

mdboom added the needs_review label Apr 14, 2016

tacaswell added this to the 2.1 (next point release) milestone Apr 14, 2016

Owner

tacaswell commented Apr 14, 2016

Contributor

TrishGillett commented Apr 14, 2016

The failures on Travis are legit, some pep8 (forgot to pylint the last couple files I worked on) and apparently my polys3d_demo.py throws an error on the Travis environment that isn't thrown on my computer. I'll sort these out tonight or tomorrow.

Contributor

TrishGillett commented Apr 15, 2016

The errors I mentioned are fixed now.

By the way, I haven't changed any file names yet but as I understand it the '_demo' should be dropped from all names and files that are only differentiated by numbers should get more descriptive names. I don't have any good ideas for renaming the five contour examples, any suggestions?

@WeatherGod WeatherGod and 1 other commented on an outdated diff Apr 15, 2016

examples/mplot3d/contour3d_demo3.py
cset = ax.contour(X, Y, Z, zdir='x', offset=-40, cmap=cm.coolwarm)
cset = ax.contour(X, Y, Z, zdir='y', offset=40, cmap=cm.coolwarm)
+cset = ax.contour(X, Y, Z, zdir='z', offset=-100, cmap=cm.coolwarm)
@WeatherGod

WeatherGod Apr 15, 2016

Member

Probably should double-check that this doesn't make an graphing artifacts worse. I am pretty sure I put this one first for a reason...

@TrishGillett

TrishGillett Apr 15, 2016

Contributor

Do you mean that it's better to plot the z one first because it's on the 'bottom' from the perspective of the default angle, or something else? And if so, do you think that choice warrants a comment?

@WeatherGod

WeatherGod Apr 15, 2016

Member

In some cases, yes. It is what I call the "Escher Effect". When the 3d bounding box of an artist (as oriented in the viewer's coordinate system) intersects with the 3d bounding box of another artist, the z-order trick that mplot3d uses becomes insufficient. Plotting some things before others can sometimes help, but not always. I don't know how much you'd want to go into it in these examples. I do cover it a bit in mplot3d's FAQ.

@TrishGillett

TrishGillett Apr 15, 2016

Contributor

Right, I have seen that kind of Escher effect at least once... when I was working on hist3d_demo I toyed with bar sizes and noticed that things got really weird when multiple bars overlapped. I'm good with changing the order back and not writing a comment, if users try another ordering and encounter trouble then it's likely that either they'll guess how to fix it or they'll be able to google it and find out.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Apr 15, 2016

examples/mplot3d/contourf3d_demo2.py
@@ -1,7 +1,9 @@
-"""
-.. versionadded:: 1.1.0
- This demo depends on new features added to contourf3d.
-"""
+'''
+Demonstrates displaying a 3D surface while also projecting filled contour
+'profiles' onto the 'walls' of the graph.
+
+See contourf3d_demo2 for the unfilled version.
@WeatherGod

WeatherGod Apr 15, 2016

Member

I think you mean contour3d_demo2.

@TrishGillett

TrishGillett Apr 15, 2016

Contributor

Yes, typo.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Apr 15, 2016

examples/mplot3d/mixed_subplots_demo.py
X, Y = np.meshgrid(X, Y)
R = np.sqrt(X**2 + Y**2)
Z = np.sin(R)
surf = ax.plot_surface(X, Y, Z, rstride=1, cstride=1,
linewidth=0, antialiased=False)
-
ax.set_zlim3d(-1, 1)
@WeatherGod

WeatherGod Apr 15, 2016

Member

could we update this to be set zlim(-1, 1) instead? The "3d" part is an anachronism.

@TrishGillett

TrishGillett Apr 15, 2016

Contributor

Sounds good about this and the other instances of set_{x,y,z}lim3d. Honestly, I'm teaching myself about matplotlib's 3D plotting by working on these examples, so I didn't know about that.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Apr 15, 2016

examples/mplot3d/polys3d_demo.py
from mpl_toolkits.mplot3d import Axes3D
from matplotlib.collections import PolyCollection
from matplotlib.colors import colorConverter
import matplotlib.pyplot as plt
import numpy as np
+def cc(arg):
+ '''
+ Shorthand to convert 'named' colours to rgba format at 60% opacity.
@WeatherGod

WeatherGod Apr 15, 2016

Member

"colours" -> "colors"

@TrishGillett

TrishGillett Apr 15, 2016

Contributor

Oops, my Canadian is showing. ;)

@WeatherGod WeatherGod commented on an outdated diff Apr 15, 2016

examples/mplot3d/polys3d_demo.py
ax.set_zlabel('Z')
+ax.set_xlim3d(0, 10)
+ax.set_ylim3d(-1, 4)
@WeatherGod

WeatherGod Apr 15, 2016

Member

again, let's clean up the usage of set_xlim3d to set_xlim() and such.

Contributor

TrishGillett commented Apr 16, 2016

All discussed changes are incorporated, I amended them into their corresponding commits.

@WeatherGod WeatherGod merged commit f00db67 into matplotlib:master Apr 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mdboom removed the needs_review label Apr 19, 2016

Member

WeatherGod commented Apr 19, 2016

Thanks for your efforts. Cleanups are always welcomed!

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