Opened 4 years ago

Closed 4 years ago

#23776 closed defect (fixed)

arctan2 for RDF causes plotting errors

Reported by: novoselt Owned by:
Priority: major Milestone: sage-8.1
Component: symbolics Keywords:
Cc: Merged in:
Authors: Andrey Novoseltsev Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 3bdda38 (Commits, GitHub, GitLab) Commit: 3bdda389ef5356befe94c8f7586e9ffc650d9901
Dependencies: Stopgaps:

Status badges

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

Change History (8)

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:

3bdda38Reality 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.