region_plot does not respect the passed variable order

[jasongrout]

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.

Component: graphics

Author: Jason Grout

Reviewer: Karl-Dieter Crisman

Merged: sage-4.3.1.alpha2

Issue created by migration from https://trac.sagemath.org/ticket/7809

[jasongrout]

comment:1

Attachment: trac-7809-simplify-equify.patch.gz

[kcrisman]

comment:2

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

[jasongrout]

comment:3

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.

[kcrisman]

comment:4

Replying to @jasongrout:

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.

[jasongrout]

comment:5

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!

[kcrisman]

comment:6

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

[jasongrout]

comment:7

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.

[kcrisman]

comment:8

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.

[jasongrout]

comment:9

Attachment: plot.png

Oh, you're right. It is correct.

Well, I just rewrote the mangle_neg part anyway. I'll attach a patch in a second.

[jasongrout]

comment:10

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

[jasongrout]

apply on top of previous patch

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

Author: Jason Grout

[kcrisman]

comment:11

Attachment: trac-7809-simplify-negative-code.patch.gz

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.

[jasongrout]

comment:12

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

[jasongrout]

Attachment: trac-7809-variable-order.patch.gz

apply on top of previous patch

[jasongrout]

comment:13

Okay, I made the changes you requested to the doctests and attached a patch. Can you mark this as positively reviewed?

[kcrisman]

comment:15

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.

[jasongrout]

comment:16

This ticket also fixes #5885, so that should be closed when this is.

(the deprecation warning is now printed).

[rlmill]

Merged: 4.3.1.alpha2

[sagetrac-mvngu]

Changed merged from 4.3.1.alpha2 to sage-4.3.1.alpha2