Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

carrutstick
Copy link

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 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.
@dmcdougall
Copy link
Member

I like this.

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

@WeatherGod
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@WeatherGod
Copy link
Member

That would work fine. I have been avoiding rcparams for mplot3d for a while
but maybe it is time to decide on a naming scheme?

@dmcdougall
Copy link
Member

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

@efiring
Copy link
Member

efiring commented Apr 27, 2013

@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
Copy link
Member

pelson commented May 13, 2013

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 May 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants