Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

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

trac-7809-simplify-equify.patch (2.3 KB) - added by jason 11 years ago.
plot.png (12.5 KB) - added by kcrisman 11 years ago.
trac-7809-simplify-negative-code.patch (2.4 KB) - added by jason 11 years ago.
apply on top of previous patch
trac-7809-variable-order.patch (1.3 KB) - added by jason 11 years ago.
apply on top of previous patch

Download all attachments as: .zip

Change History (22)

Changed 11 years ago by jason

comment:1 Changed 11 years ago by jason

  • Status changed from new to needs_review

comment:2 follow-ups: Changed 11 years ago by kcrisman

  • Status changed from needs_review to needs_work

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)

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

comment:3 in reply to: ↑ 2 ; follow-up: Changed 11 years ago by 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.

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 kcrisman

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

Actually, the second example above produces the wrong image even after the patch is applied!

comment:6 Changed 11 years ago by kcrisman

Are you sure? Are we always putting x on the horizontal axis? This seems okay to me.

comment:7 Changed 11 years ago by jason

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 kcrisman

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 kcrisman

comment:9 Changed 11 years ago by jason

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 jason

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

Changed 11 years ago by jason

apply on top of previous patch

comment:11 follow-up: Changed 11 years ago by kcrisman

  • Authors set to Jason Grout
  • 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 jason

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

Changed 11 years ago by jason

apply on top of previous patch

comment:13 Changed 11 years ago by jason

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 jason

  • Status changed from needs_work to needs_review

comment:15 Changed 11 years ago by kcrisman

  • 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 jason

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 rlm

  • 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 mvngu

  • Merged in changed from 4.3.1.alpha2 to sage-4.3.1.alpha2
Note: See TracTickets for help on using tickets.