Slightly concerned about making the API too complicated.

Perhaps better as a single "max_gridsize" kwarg?

Here is my rationale for my API decision. The rest of the plot_surface() method is highly dependent upon rstride/cstride. It would be a significant amount of work to refactor that to take something more general, and yield very little gain. So, if rstride/cstride are still going to be the critical pieces of information within the method, then it makes no sense to deprecate them as inputs... so they stay.

Next, I dislike the idea of overloading the current rstride/cstride arguments, which has been suggested by others. The names of the arguments have very specific and clear meaning, and so they would be difficult to conceptually overload. Besides, what would I overload it with? String representation of ints? That would be awkward and tedious.

So, that means that not only are rstride/cstride are going to stay, but that new arguments will be needed for any new specification. Given the orthogonality of rstride/cstride, it makes sense that the new arguments would also be orthogonal (so, have two arguments instead of just one). It also makes it simple for managing precedence if both or neither the stride and the new argument are specified.

So, all that is left is the nature of the new arguments. I like my choice of "maximum count" instead of "exact count" because I can't guarantee that the array could be split into that many equally spaced pieces. Remember, I still have to plug into the cstride/rstride usage internally, so the samples must be equally spaced.

So, now the user is free to use either a stride or a count, which is conceptually similar to arange() vs. linspace() (granted, the max count idea doesn't fit perfectly with linspace). Depending on their situation, they are free to choose whichever approach that best suits them.

I am not entirely sold on the need to apply a "classic mode" as fully as I do here. I could be convinced to only apply it if neither stride nor count is given.

@WeatherGod I am convinced and am ok with this going in as-is. Slight preference for raising if both stride and count are given which would make it a bit easier to document and no need to have the classic compatibility mode.

I agree that passing both flags is an error which shouldn't pass silently.

Agreed. I'll see if I can find time tonight to fix that.

On Mon, Oct 3, 2016 at 3:30 PM, Ryan May notifications@github.com wrote:

I agree that passing both flags is an error which shouldn't pass silently.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7164 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-CRrdv8BDB7nvgr50pIvbBahQEAUks5qwVfQgaJpZM4KEX1M
.

Needs a rebase now.

Just realized something... I should add this feature to plot_wireframe(), too, right?

Of course, the question then becomes: if we do add it to plot_wireframe(), then should rcount/ccount be the default samplers, or should it remain rstride/cstride (which currently defaults to 1, unlike plot_surface())?

I am 👍 on adding it to plot_wireframe if we can get it done in time. That should not hold this PR.

Failures look real and related to this PR.

Yes, I forgot to update the tests when I made it such that you can't
specify both counts and strides. I have that fix in queue. I am just trying
to figure out what is desired for plot_wireframe() because it is similar,
but not the same as plot_surface()'s argument handling.

On Thu, Nov 10, 2016 at 10:03 PM, Thomas A Caswell <notifications@github.com

wrote:

I am 👍 on adding it to plot_wireframe if we can get it done in time.
That should not hold this PR.

Failures look real and related to this PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7164 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-DTB94imZ71-tOMO4As3p6Pj224Hks5q89sRgaJpZM4KEX1M
.

bump @WeatherGod

The single backticks should be double backticks in many cases; they aren't references, but code.

Perhaps, but that'll be more suited for a different PR. Right now, I am just following the current conventions in this file.

@WeatherGod Why did you regenerate the pdf and svg test images?

yeah, looks like those images were accidentally regenerated. I brought back what was in master for them and re-ran my tests and they passed. So I committed those images back in, and then did a squash commit to eliminate the change out of the history. The tests should be done shortly.