Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make 'rstride', 'cstride' default values smarter. #1040

Closed
wants to merge 1 commit into from

5 participants

@carrutstick

This commit is a minor revision that may reduce headaches for new users.

Previously, the default behavior for Axes3D.plot_surface resulted in very confusing plots for data of size less than 10X10. Additionally, inadvertently plotting very large matrices resulted in too many polygons being drawn, such that the lines along their edges obscure the polygons themselves.

This commit changes default behavior to draw as many polygons as possible not exceeding 100 to a row or column.

@carrutstick carrutstick Make 'rstride', 'cstride' default values smarter
This commit is a minor revision that may reduce headaches for new users. 

Previously, the default behavior for Axes3D.plot_surface resulted in very confusing plots for data of size less than 10X10. Additionally, inadvertently plotting very large matrices resulted in too many polygons being drawn, such that the lines along their edges obscure the polygons themselves.

This commit changes default behavior to draw as many polygons as possible not exceeding 100 to a row or column.
10ea71f
@dmcdougall
Collaborator

I like this.

@WeatherGod Since this is not invasive, would it be possible to merge this before the Axes3D refactor?

@WeatherGod
Collaborator

I am not a fan of this change. There really isn't any precedence that I know of for this type of default behavior. For example, histogram has a default fixed number of bins to utilize. OTOH, there have been many complaints that matplotlib should provide better/more intelligent default behaviors.

I will have to mull this over a little more, and I would welcome input from other devs as well. In the meantime, at the very least, we need the doc strings to be updated to reflect this. I am also a little hesitant to change an existing default behavior on users, however mplot3d has always been in flux, so this likely not to be a deal breaker since the only noticeable change is a slight visual impact.

As for the Axes3D refactor, don't worry about that, it will not happen for the upcoming release.

@dmcdougall
Collaborator

I think the comparison to the default number of bins in a histogram plot is a little unfair, especially when one considers the fact that the reason a user would change the number of bins is to analyse the behaviour of a sample set depending on how it is partitioned. This is different from changing the rstride and cstride parameters here. A user wouldn't change them for the purpose of scientific analysis, they would change it to resolve more of the surface in the plot, or even resolve less of the surface, thereby speeding up the plotter. I think a more clever default value depending on the size of the input is a sensible idea. That's just my two pence, though.

@dmcdougall
Collaborator

Perhaps an rcparam for the stride would be better? Thoughts?

@WeatherGod
Collaborator
@dmcdougall
Collaborator

@WeatherGod Perhaps axes3d.surface.rstride and axes3d.surface.cstride?

@efiring
Owner

@weathergod, it looks to me like maybe you should close this PR, since it has been inactive for 6 months, it doesn't appear likely to be merged in its present form, and it is entirely in your mplot3d domain. It would be good to reduce the clutter in the PR list.

@pelson
Collaborator

It would be good to reduce the clutter in the PR list.

Agreed. @carrutstick - I'm closing this because there isn't a general agreement about this change. It does appear that the fix could be achieved after a discussion on mpl-devel, potentially followed by a MEP (https://github.com/matplotlib/matplotlib/wiki#matplotlib-enhancement-proposals-meps) to add mplot3d rcParameters. It would be amazing if you were willing to drive that discussion along with input from @WeatherGod.

@pelson pelson closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 25, 2012
  1. @carrutstick

    Make 'rstride', 'cstride' default values smarter

    carrutstick authored
    This commit is a minor revision that may reduce headaches for new users. 
    
    Previously, the default behavior for Axes3D.plot_surface resulted in very confusing plots for data of size less than 10X10. Additionally, inadvertently plotting very large matrices resulted in too many polygons being drawn, such that the lines along their edges obscure the polygons themselves.
    
    This commit changes default behavior to draw as many polygons as possible not exceeding 100 to a row or column.
This page is out of date. Refresh to see the latest.
Showing with 5 additions and 5 deletions.
  1. +5 −5 lib/mpl_toolkits/mplot3d/axes3d.py
View
10 lib/mpl_toolkits/mplot3d/axes3d.py
@@ -1362,8 +1362,8 @@ def plot_surface(self, X, Y, Z, *args, **kwargs):
X.shape = (rows, cols)
Y.shape = (rows, cols)
- rstride = kwargs.pop('rstride', 10)
- cstride = kwargs.pop('cstride', 10)
+ rstride = kwargs.pop('rstride', max(1, rows/100))
+ cstride = kwargs.pop('cstride', max(1, cols/100))
if 'facecolors' in kwargs:
fcolors = kwargs.pop('facecolors')
@@ -1520,9 +1520,6 @@ def plot_wireframe(self, X, Y, Z, *args, **kwargs):
Returns a :class:`~mpl_toolkits.mplot3d.art3d.Line3DCollection`
'''
- rstride = kwargs.pop("rstride", 1)
- cstride = kwargs.pop("cstride", 1)
-
had_data = self.has_data()
Z = np.atleast_2d(Z)
# FIXME: Support masked arrays
@@ -1535,6 +1532,9 @@ def plot_wireframe(self, X, Y, Z, *args, **kwargs):
X.shape = (rows, cols)
Y.shape = (rows, cols)
+ rstride = kwargs.pop("rstride", max(1, rows/100))
+ cstride = kwargs.pop("cstride", max(1, cols/100))
+
# We want two sets of lines, one running along the "rows" of
# Z and another set of lines running along the "columns" of Z.
# This transpose will make it easy to obtain the columns.
Something went wrong with that request. Please try again.