#7809 closed defect (fixed)
region_plot does not respect the passed variable order
Reported by: | jason | Owned by: | was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.3.1 |
Component: | graphics | Keywords: | |
Cc: | Merged in: | sage-4.3.1.alpha2 | |
Authors: | Jason Grout | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The call region_plot(2/x + 1/y > 1/x * 1/y, (x,-10,10), (y,-10,10))
passes the following function to setup_for_eval_on_grid: (y, x) |--> -2/x - 1/y + 1/(x*y)
, but passes the variables in the order (x,y). The problem is the equify function. This patch simplifies the code in equify to not try to put an ordering on the variables, but to just pass back an expression (not a function).
In practice, since variables would be substituted by name, I don't think this will make a difference. But it does make the code cleaner and more correct.
Attachments (4)
Change History (22)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 follow-ups: ↓ 3 ↓ 5 Changed 11 years ago by
- Status changed from needs_review to needs_work
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 11 years ago by
Replying to kcrisman:
Looks like this is the only place equify is used, so I think this does not require any deprecation for the removed optional argument. There should be another doctest in to show this works, though, like
sage: region_plot([y>.3, x>0, x^2+y^2<1], (x,-1.1, 1.1), (y,-1.1, 1.1), plot_points = 400).show(aspect_ratio=1) sage: region_plot([y>.3, x>0, x^2+y^2<1], (y,-1.1, 1.1), (x,-1.1, 1.1), plot_points = 400).show(aspect_ratio=1)
Does this actually produce an incorrect plot before the patch? I'll check, but I'm pretty sure it should work.
Also, the example in the description fails! Though, to be fair, it failed before as well - but I figure I should mention it in case it's related to this ticket after all. Or was the syntax wrong?
sage: region_plot(2/x + 1/y > 1/x * 1/y, (x,-10,10), (y,-10,10)) TypeError: reduce() of empty sequence with no initial value
This is not related to this ticket. The error is caused by a bug in fast_callable--see #7810.
comment:4 in reply to: ↑ 3 Changed 11 years ago by
Replying to jason:
Replying to kcrisman:
Looks like this is the only place equify is used, so I think this does not require any deprecation for the removed optional argument. There should be another doctest in to show this works, though, like
sage: region_plot([y>.3, x>0, x^2+y^2<1], (x,-1.1, 1.1), (y,-1.1, 1.1), plot_points = 400).show(aspect_ratio=1) sage: region_plot([y>.3, x>0, x^2+y^2<1], (y,-1.1, 1.1), (x,-1.1, 1.1), plot_points = 400).show(aspect_ratio=1)Does this actually produce an incorrect plot before the patch? I'll check, but I'm pretty sure it should work.
I didn't bother to check, but it seems like this was the concern, or? At any rate there should be something documented that didn't work before and now does. Otherwise I don't quite get why we are removing the potential for passing in the variables here.
comment:5 in reply to: ↑ 2 Changed 11 years ago by
Replying to kcrisman:
Looks like this is the only place equify is used, so I think this does not require any deprecation for the removed optional argument. There should be another doctest in to show this works, though, like
sage: region_plot([y>.3, x>0, x^2+y^2<1], (x,-1.1, 1.1), (y,-1.1, 1.1), plot_points = 400).show(aspect_ratio=1) sage: region_plot([y>.3, x>0, x^2+y^2<1], (y,-1.1, 1.1), (x,-1.1, 1.1), plot_points = 400).show(aspect_ratio=1)
Actually, the second example above produces the wrong image even after the patch is applied!
comment:6 Changed 11 years ago by
Are you sure? Are we always putting x on the horizontal axis? This seems okay to me.
comment:7 Changed 11 years ago by
no, x shouldn't be on the horizontal axis always. The first variable specified should be on the horizontal axis. That would then be consistent.
comment:8 Changed 11 years ago by
Right, and in the second plot the first variable is on the horizontal axis - see attached graphic. By the way, note the one-pixel issue still - aargh! I wonder what the heck is causing that.
Changed 11 years ago by
comment:9 Changed 11 years ago by
Oh, you're right. It is correct.
Well, I just rewrote the mangle_neg part anyway. I'll attach a patch in a second.
comment:10 Changed 11 years ago by
Before the simplify-negative-code:
sage: %time region_plot([y>.3, x>0, x^2+y^2<1], (y,-1.1, 1.1), (x,-.5, 1.1)).show(aspect_ratio=1) CPU times: user 1.96 s, sys: 0.08 s, total: 2.04 s Wall time: 2.38 s sage: %time region_plot([y>.3, x>0, x^2+y^2<1], (y,-1.1, 1.1), (x,-.5, 1.1),plot_points=400).show(aspect_ratio=1) CPU times: user 5.92 s, sys: 0.08 s, total: 5.99 s Wall time: 6.30 s
After:
sage: %time region_plot([y>.3, x>0, x^2+y^2<1], (y,-1.1, 1.1), (x,-.5, 1.1)).show(aspect_ratio=1) CPU times: user 1.27 s, sys: 0.02 s, total: 1.29 s Wall time: 1.36 s sage: %time region_plot([y>.3, x>0, x^2+y^2<1], (y,-1.1, 1.1), (x,-.5, 1.1),plot_points=400).show(aspect_ratio=1) CPU times: user 2.49 s, sys: 0.04 s, total: 2.53 s Wall time: 2.67 s
comment:11 follow-up: ↓ 12 Changed 11 years ago by
- Reviewers set to Karl-Dieter Crisman
Too bad about mangle_neg, but it was almost more confusing that way than in the code, I think you are right.
Be sure to include some test where the order of coordinates is switched. Incidentally, you should also remove the #long time flag from that one test; it only takes one second on my machine, which I don't think counts as a long time any more. The file takes nearly a half minute to test for me, though!
Other than that, positive review.
comment:12 in reply to: ↑ 11 Changed 11 years ago by
Replying to kcrisman:
Be sure to include some test where the order of coordinates is switched.
The old code gave the correct result too, I think. I consider this patch more a refactoring of code. The error that I corrected didn't show up because I think we were more careful in another part of the code.
Jason
comment:13 Changed 11 years ago by
Okay, I made the changes you requested to the doctests and attached a patch. Can you mark this as positively reviewed?
comment:14 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:15 Changed 11 years ago by
- Status changed from needs_review to positive_review
Looks good. The use of s and t is good because then it's not so clear to the user from convention which one "should" be horizontal. Positive review; apply in the order simplify-equify, simplify-negative-code, variable-order.
comment:16 Changed 11 years ago by
This ticket also fixes #5885, so that should be closed when this is.
(the deprecation warning is now printed).
comment:17 Changed 11 years ago by
- Merged in set to 4.3.1.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
comment:18 Changed 11 years ago by
- Merged in changed from 4.3.1.alpha2 to sage-4.3.1.alpha2
Looks like this is the only place equify is used, so I think this does not require any deprecation for the removed optional argument. There should be another doctest in to show this works, though, like
Also, the example in the description fails! Though, to be fair, it failed before as well - but I figure I should mention it in case it's related to this ticket after all. Or was the syntax wrong?