Opened 11 years ago

Closed 11 years ago

# region_plot does not respect the passed variable order

Reported by: Owned by: jason was major sage-4.3.1 graphics sage-4.3.1.alpha2 Jason Grout Karl-Dieter Crisman N/A

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

### comment:1 Changed 11 years ago by jason

• Status changed from new to needs_review

### comment:2 follow-ups: ↓ 3 ↓ 5 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: ↓ 4 Changed 11 years ago by jason

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

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

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.

### 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: ↓ 12 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

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.