same range variables -- bug in 3d plotting (probably very very very easy to fix)

[williamstein]

sage: var('x,y')
sage: plot3d(sin(x*y), (x,-1,1), (x,-1,1))
boom!

The problem is that both ranges use the variable x. The fix is to make sure that the two variables are different and if not raise an exception (do this also in parametric_plot3d).

Component: graphics

Keywords: editor_mhansen

Issue created by migration from https://trac.sagemath.org/ticket/1877

[687b86f2-44be-4630-8c84-2261734ed4dd]

comment:1

Attachment: 1877fix.hg.gz

plot3d and parametric_plot3d should fail a tiny bit more gracefully if they're given two ranges using the same variable:

sage: var('x,y')
sage: plot3d(sin(x*y), (x,-1,1), (x,-1,1))
ValueError: If the ranges in the argument of plot3d are 3-tuples, then the first components of those 3-tuples must be different.
sage: var('u,v')
sage: parametric_plot3d((cos(u), sin(u) + cos(v), sin(v)), (u, 0, 2*pi), (u, -pi, pi))
ValueError: If the ranges in the argument of parametric_plot3d are both 3-tuples, then the first components of those 3-tuples must be different.

[85eec1a4-3d04-4b4d-b711-d4db03337c41]

Changed keywords from none to editor_mhansen

[robertwb]

comment:4

Please give a message with the raised value error. Pending that, I'd give a positive review.

[e0d4dd3a-58d6-47e6-b509-f9cf71bc0f0a]

comment:5

Attachment: trac_1877.patch.gz

thomag, your patch is along the right lines, but the implementation wasn't quite right. You don't need to catch an error after raising it and then print something to the screen. Just supply a message with the error, and it will show up unless it's caught somewhere else. Also, when you fix a bug, you should add a doctest demonstrating the new, correct behavior.

I've posted a new patch that raises an error in the usual way, and provides a briefer but still clear error message. If this is accepted, only trac_1877.patch should be applied.

[5244866e-e17b-4731-a0e2-6dffffcacd5f]

comment:6

Attachment: trac_1877_review.patch.gz

trac_1877_review.patch does some minor cosmetic adjustements on top of trac_1877.patch (like not starting the error messages with a capital). Otherwise this gets a positive review from me.

It might still my part still needs to be reviewed so I'll leave it as needs review.

[e0d4dd3a-58d6-47e6-b509-f9cf71bc0f0a]

comment:7

Your review patch looks good to me, positive review. Both trac_1877.patch and trac_1877_review.patch should be applied

[e0d4dd3a-58d6-47e6-b509-f9cf71bc0f0a]

comment:8

Oops, this won't pass it's doctests as written, since the review patch alters the error message thrown, but not the doctest.

[5244866e-e17b-4731-a0e2-6dffffcacd5f]

comment:9

Shame on me. But with the new patch the doctests are changed and they pass.

[5244866e-e17b-4731-a0e2-6dffffcacd5f]

Attachment: trac_1877_doctests.patch.gz

[85eec1a4-3d04-4b4d-b711-d4db03337c41]

comment:11

Merged trac_1877.patch, trac_1877_review.patch and trac_1877_doctests.patch in Sage 3.1.2.rc0.