Ticket #1877 (closed defect: fixed)

Opened 11 months ago

Last modified 3 months ago

[with patch, positive review] same range variables -- bug in 3d plotting (probably very very very easy to fix)

Reported by: was Assigned to: thomag
Priority: major Milestone: sage-3.1.2
Component: graphics Keywords: editor_mhansen
Cc:

Description 

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).

Attachments

1877fix.hg (1.2 kB) - added by thomag on 08/26/2008 08:37:48 AM.
trac_1877.patch (2.5 kB) - added by jwmerrill on 09/02/2008 04:08:26 PM.
trac_1877_review.patch (1.3 kB) - added by anakha on 09/06/2008 08:39:47 AM.
trac_1877_doctests.patch (1.3 kB) - added by anakha on 09/06/2008 09:14:40 AM.

Change History

08/26/2008 08:37:48 AM changed by thomag

  • attachment 1877fix.hg added.

08/26/2008 08:52:31 AM changed by thomag

  • owner changed from was to thomag.
  • status changed from new to assigned.

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.

08/26/2008 09:04:58 AM changed by thomag

  • summary changed from same range variables -- bug in 3d plotting (probably very very very easy to fix) to [with patch, needs review] same range variables -- bug in 3d plotting (probably very very very easy to fix).

08/28/2008 05:32:07 PM changed by mabshoff

  • keywords set to editor_mhansen.

09/02/2008 01:39:02 AM changed by robertwb

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

09/02/2008 04:08:26 PM changed by jwmerrill

  • attachment trac_1877.patch added.

09/02/2008 04:15:50 PM changed by jwmerrill

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.

09/06/2008 08:39:47 AM changed by anakha

  • attachment trac_1877_review.patch added.

09/06/2008 08:43:29 AM changed by anakha

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.

09/06/2008 08:59:25 AM changed by jwmerrill

  • summary changed from [with patch, needs review] same range variables -- bug in 3d plotting (probably very very very easy to fix) to [with patch, positive review] same range variables -- bug in 3d plotting (probably very very very easy to fix).

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

09/06/2008 09:02:45 AM changed by jwmerrill

  • summary changed from [with patch, positive review] same range variables -- bug in 3d plotting (probably very very very easy to fix) to [with patch, needs work] same range variables -- bug in 3d plotting (probably very very very easy to fix).

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

09/06/2008 09:14:28 AM changed by anakha

  • summary changed from [with patch, needs work] same range variables -- bug in 3d plotting (probably very very very easy to fix) to [with patch, needs review] same range variables -- bug in 3d plotting (probably very very very easy to fix).

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

09/06/2008 09:14:40 AM changed by anakha

  • attachment trac_1877_doctests.patch added.

09/06/2008 09:17:50 AM changed by jwmerrill

  • summary changed from [with patch, needs review] same range variables -- bug in 3d plotting (probably very very very easy to fix) to [with patch, positive review] same range variables -- bug in 3d plotting (probably very very very easy to fix).

09/06/2008 04:12:27 PM changed by mabshoff

  • status changed from assigned to closed.
  • resolution set to fixed.

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