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: |
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)
Change History (15)
Changed 13 years ago by
comment:1 Changed 13 years ago by
- Owner changed from was to thomag
- Status changed from new to assigned
comment:2 Changed 13 years ago by
- 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
- Keywords editor_mhansen added
comment:4 Changed 13 years ago by
Please give a message with the raised value error. Pending that, I'd give a positive review.
Changed 13 years ago by
comment:5 Changed 13 years ago by
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
comment:6 Changed 13 years ago by
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
- 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
- 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
- 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
comment:10 Changed 13 years ago by
- 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
- 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.
plot3d and parametric_plot3d should fail a tiny bit more gracefully if they're given two ranges using the same variable: