Opened 4 years ago

Closed 4 years ago

# arctan2 for RDF causes plotting errors

Reported by: Owned by: novoselt major sage-8.1 symbolics Andrey Novoseltsev Ralf Stephan N/A 3bdda38 3bdda389ef5356befe94c8f7586e9ffc650d9901

### Description

Carlo Verschoor reports that this is broken in Sage 8.0

```cones = [Cone([(0,1),(1,0)]),Cone([(-2,-1),(-1,-3)]),Cone([(0,1),(-2,-1)]),Cone([(-1,-3),(1,0)])]
F = Fan(cones)
F.plot()
```

due to `float(arctan2(RDF(-3),RDF(-1)))` having an imaginary component

### comment:1 Changed 4 years ago by rws

The problem is that we are chasing parents in https://github.com/sagemath/sage/blob/master/src/sage/libs/pynac/pynac.pyx#L1829

A naive fix would be

```diff --git a/src/sage/libs/pynac/pynac.pyx b/src/sage/libs/pynac/pynac.pyx
index acc1424188..b08dca2285 100644
--- a/src/sage/libs/pynac/pynac.pyx
+++ b/src/sage/libs/pynac/pynac.pyx
@@ -1819,6 +1819,7 @@ cdef py_atan2(x, y):
"""
from sage.symbolic.constants import pi, NaN
from sage.rings.real_arb import RealBallField
+    from sage.rings.real_double import RealDoubleField_class
from sage.rings.real_mpfr import RealField_class
P = coercion_model.common_parent(x, y)
is_real = False
@@ -1826,6 +1827,7 @@ cdef py_atan2(x, y):
P = RR
if (P is float
or parent(P) is RealField_class
+            or isinstance(P, RealDoubleField_class)
or isinstance(P, RealBallField)):
is_real = True
if y != 0:
```

but maybe there is a better way to recognize real parents and catch them all?

### comment:2 Changed 4 years ago by rws

• Component changed from graphics to symbolics

### comment:3 Changed 4 years ago by novoselt

To me the whole passage trying to figure out if `P` is real or not looks wrong and error prone (and `parent(P)` given that `P` is common parent is confusing). And presumably the question of whether to work over reals or not may arise in other situations, so there has to be a better way to do that will work for possible future fields as well. Why not do something like `is_real = RDF.has_coerce_map_from(P)` and stop on it? Are there situations when this will not work or are there performance considerations?

### comment:4 Changed 4 years ago by novoselt

While we are at it, this is also wrongish:

```    if P is ZZ:
P = RR
```

for

```from sage.libs.pynac.pynac import py_atan2_for_doctests as py_atan2
py_atan2(-1/2, 0)
```

gives `TypeError: unable to convert pi to a rational` Not sure what is correct fix here - maybe if `P` is real, we need to check that it is possible to convert something like `RDF` or `RR` into `P` as well and if not - replace `P` with "generic reals" whatever that is?

### comment:5 Changed 4 years ago by novoselt

• Branch set to u/novoselt/23776

### comment:6 Changed 4 years ago by novoselt

• Authors set to Andrey Novoseltsev
• Commit set to 3bdda389ef5356befe94c8f7586e9ffc650d9901
• Status changed from new to needs_review

Using the check that `P` coerces into `RR` seems to work.

New commits:

 ​3bdda38 `Reality check for atan2`

### comment:7 Changed 4 years ago by rws

• Reviewers set to Ralf Stephan
• Status changed from needs_review to positive_review

Thanks. Looking good, patchbot testing passes.

### comment:8 Changed 4 years ago by vbraun

• Branch changed from u/novoselt/23776 to 3bdda389ef5356befe94c8f7586e9ffc650d9901
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.