plot_surface docstring + edge case fix #9277

anntzer · 2017-10-04T01:37:56Z

PR Summary

Update doctrings of plot_surface & plot_wireframe.

Emphasize the downsampling. Clarify when the defaults for rstride and
cstride apply.

Allow passing infinite rcount & ccount to plot_surface, plot_wireframe.

Passing an infinite rcount and ccount is the easiest way to ensure the
data does not get downsampled (which I still think is a misfeature, but
that's a battle for another day), but would result in a stride of zero;
instead, ensure that the stride is at least 1.

Closes #8706.

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

tacaswell · 2017-10-04T07:05:55Z

lib/mpl_toolkits/mplot3d/axes3d.py

-        '''
+            The *rcount* and *ccount* kwargs, which both default to 50,
+            determine the maximum number of samples used in each direction.  If
+            the input data is larger, it will be downsampled to these numbers


might be worth saying 'downsampled by slicing' ?

... I would have thought it was downsampled by averaging (well, perhaps not).
This behavior is seriously ridiculous. Anyways, fixed the doc accordingly.

WeatherGod · 2017-10-04T13:40:28Z

lib/mpl_toolkits/mplot3d/axes3d.py

-        along this direction, producing a 3D line plot rather than a
-        wireframe plot.
-        Added in v2.0.0.
+        .. warning::


Why are you making this a warning?

WeatherGod · 2017-10-04T13:44:56Z

"""
(which I still think is a misfeature, but
that's a battle for another day)
"""
The downsampling is not a misfeature, so please, do not treat it as such in the documentation. Instead, emphasize the fact that one could pass infinity to avoid any automated downsampling.

WeatherGod · 2017-10-04T13:48:16Z

lib/mpl_toolkits/mplot3d/axes3d.py

-        This is the default sampling method unless using the 'classic'
-        style. Will raise ValueError if both stride and count are
-        specified.
-        Added in v2.0.0.


The fact that this kwarg was added in v2.0.0 is important. I can't tell you the number of times I run into situations where I tried using a feature in an older codebase only to discover that the feature isn't available in the older version of the package because it wasn't documented as such on the website.

WeatherGod · 2017-10-04T13:49:48Z

lib/mpl_toolkits/mplot3d/axes3d.py

-        sample the input data to generate the graph.  If 1k by 1k
-        arrays are passed in, the default values for the strides will
-        result in a 100x100 grid being plotted. Defaults to 10.
-        Raises a ValueError if both stride and count kwargs


You are also lost the information that the stride and count kwargs can't be used together, and will raise a ValueError.

anntzer · 2017-10-04T16:38:05Z

I'm not particularly interested in relitigating #3142, but the main point is that (regardless of whether it is a good design, and as reported in #8706) it is rathe unintuitive that a function defaults to silently discarding some of the input data (well, the only other example I can think of in mpl is #8278, where I took the same side of the discussion), and this kind of behavior should be emphasized in the docs.

The fact that the count and stride arguments are mutually exclusive with each other is, well, documented in the parameter list next to rstride, cstride (I don't think mentioning the exception class is particularly relevant here, if you are so careful that you want to wrap the thing in a try... except you may as well not pass count and stride at the same time.

grepping for versionadded/versionchanged in the codebase shows that it is exclusively used in mplot3d (+ 1 example, + 1 docstring of a function I backported from numpy). And even there, basically no change has been logged between 1.2 and 2.0 (rcount/ccount where added in 2.0, then voxels in 2.1), which suggests that there is probably a lot of missing version information anyways. Let's just stop pretending we're keeping that information available and up to date.

jklymak · 2017-10-04T17:06:00Z

I was mystified by this default behaviour once upon a time as well. It is indeed obvious that matplotlib is doing something to the data. On the other hand, its not at all clear that it is by design and that there are arguments to set the decimation, called cstride and rstride no less, which are terms I wasn't familiar with before running into this behaviour. If you aren't going to change the defaults, any documentation that highlights this behaviour in a stark way is most welcome IMHO.

WeatherGod · 2017-10-04T18:12:27Z

I am not saying that it shouldn't be highlighted. I am against putting it in as a warning. If anything, make it a note. As for mutual exclusivity, I missed it in your diff, so I take that back.

As for the versionadded thing, we aren't pretending anything. I have been meticulous in my maintenance of the versionadded info in mplot3d. There haven't been many new features added since I did my overhaul 5 or so years ago. Please don't remove useful information.

anntzer · 2017-10-04T19:58:37Z

I think the new version is a reasonable middle ground for everyone.

WeatherGod

Agreed, this is a definite improvement for the docstring. Just noticed a possible typo, though.

WeatherGod · 2017-10-04T20:01:44Z

lib/mpl_toolkits/mplot3d/axes3d.py

+        rstride, cstride : int
+            Downsampling stride in each direction.  These arguments are
+            mutually exclusive with *rcount* and *ccount*.  If only one of
+            *rcount* or *ccount* is set, the other defaults to 10.


Whoops, I think you mean If only one of *rstride* or *cstride* here.

good catch, fixed (below too)

WeatherGod · 2017-10-04T20:03:58Z

lib/mpl_toolkits/mplot3d/axes3d.py

+        rstride, cstride : int
+            Downsampling stride in each direction.  These arguments are
+            mutually exclusive with *rcount* and *ccount*.  If only one of
+            *rcount* or *ccount* is set, the other defaults to 1.  Setting a


…me}. Docstrings: Emphasize the downsampling. Clarify when the defaults for rstride and cstride apply. Passing an infinite rcount and ccount is the easiest way to ensure the data does not get downsampled (which I still think is a misfeature, but that's a battle for another day), but would result in a stride of zero; instead, ensure that the stride is at least 1.

tacaswell · 2017-10-05T07:13:10Z

@meeseeksdev backport to v2.1.x

Backport PR #9277 on branch v2.1.x

anntzer added the topic: mplot3d label Oct 4, 2017

anntzer changed the title ~~Plot surface~~ plot_surface docstring + edge case fix Oct 4, 2017

tacaswell reviewed Oct 4, 2017

View reviewed changes

tacaswell approved these changes Oct 4, 2017

View reviewed changes

tacaswell added this to the 2.1.1 (next bug fix release) milestone Oct 4, 2017

anntzer force-pushed the plot-surface branch from 5487ecd to 6f5a530 Compare October 4, 2017 07:15

WeatherGod reviewed Oct 4, 2017

View reviewed changes

anntzer force-pushed the plot-surface branch from 6f5a530 to f66875c Compare October 4, 2017 19:58

WeatherGod requested changes Oct 4, 2017

View reviewed changes

anntzer force-pushed the plot-surface branch from f66875c to de0848e Compare October 4, 2017 20:48

WeatherGod approved these changes Oct 4, 2017

View reviewed changes

WeatherGod merged commit 9cb0f74 into matplotlib:master Oct 5, 2017

anntzer deleted the plot-surface branch October 5, 2017 02:07

lumberbot-app bot mentioned this pull request Oct 5, 2017

Backport PR #9277 on branch v2.1.x #9283

Merged

lumberbot-app bot pushed a commit that referenced this pull request Oct 5, 2017

Backport PR #9277: plot_surface docstring + edge case fix

df6139b

dstansby added a commit that referenced this pull request Oct 7, 2017

Merge pull request #9283 from matplotlib/auto-backport-of-pr-9277

0493add

Backport PR #9277 on branch v2.1.x

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plot_surface docstring + edge case fix #9277

plot_surface docstring + edge case fix #9277

anntzer commented Oct 4, 2017

tacaswell Oct 4, 2017

anntzer Oct 4, 2017

WeatherGod Oct 4, 2017

WeatherGod commented Oct 4, 2017

WeatherGod Oct 4, 2017

WeatherGod Oct 4, 2017

anntzer commented Oct 4, 2017

jklymak commented Oct 4, 2017

WeatherGod commented Oct 4, 2017

anntzer commented Oct 4, 2017

WeatherGod left a comment

WeatherGod Oct 4, 2017

anntzer Oct 4, 2017

WeatherGod Oct 4, 2017

tacaswell commented Oct 5, 2017

plot_surface docstring + edge case fix #9277

plot_surface docstring + edge case fix #9277

Conversation

anntzer commented Oct 4, 2017

PR Summary

Update doctrings of plot_surface & plot_wireframe.

Allow passing infinite rcount & ccount to plot_surface, plot_wireframe.

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeatherGod commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Oct 4, 2017

jklymak commented Oct 4, 2017

WeatherGod commented Oct 4, 2017

anntzer commented Oct 4, 2017

WeatherGod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Oct 5, 2017