Opened 13 years ago

Closed 13 years ago

#1877 closed defect (fixed)

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

Reported by: was Owned by: thomag
Priority: major Milestone: sage-3.1.2
Component: graphics Keywords: editor_mhansen
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

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 (4)

1877fix.hg (1.2 KB) - added by thomag 13 years ago.
trac_1877.patch (2.5 KB) - added by jwmerrill 13 years ago.
trac_1877_review.patch (1.3 KB) - added by anakha 13 years ago.
trac_1877_doctests.patch (1.3 KB) - added by anakha 13 years ago.

Download all attachments as: .zip

Change History (15)

Changed 13 years ago by thomag

comment:1 Changed 13 years ago 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.

comment:2 Changed 13 years ago 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)

comment:3 Changed 13 years ago by mabshoff

  • Keywords editor_mhansen added

comment:4 Changed 13 years ago by robertwb

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

Changed 13 years ago by jwmerrill

comment:5 Changed 13 years ago 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.

Changed 13 years ago by anakha

comment:6 Changed 13 years ago 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.

comment:7 Changed 13 years ago 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

comment:8 Changed 13 years ago 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.

comment:9 Changed 13 years ago 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.

Changed 13 years ago by anakha

comment:10 Changed 13 years ago 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)

comment:11 Changed 13 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.