sage.rings.{real,complex}_double: Remove compile time dependency on cypari2

[mkoeppe]

... as previously done in #30022 for sage.rings.integer and sage.rings.rational.

This is to support modularization (#29705):

• in particular, for sagemath-categories (Modularization of sagelib: Break out separate packages sagemath-objects, sagemath-categories #29865), to make the functions py_scalar_parent and py_scalar_to_element (from sage.structure.coerce) fully functional without having to pull in PARI (at compile time or run time)
• likewise, for sagemath-polyhedra (Modularization of sagelib: Break out a separate package sagemath-polyhedra #32432) to support floating-point polyhedra and linear programming without having to pull in PARI (at compile time or run time).

CC: @kliem @videlec @tscrim @yyyyx4

Component: refactoring

Author: Matthias Koeppe, Jonathan Kliem

Branch/Commit: 629c319

Reviewer: Matthias Koeppe, Travis Scrimshaw

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

[mkoeppe]

Branch: u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2

[mkoeppe]

Author: Matthias Koeppe, ...

[mkoeppe]

comment:3

Here's an attempt but I'm getting doctest errors:

sage -t --random-seed=0 src/sage/rings/polynomial/polynomial_element.pyx
      File "sage/rings/complex_double.pyx", line 387, in sage.rings.complex_double.ComplexDoubleField_class._element_constructor_ (build/cythonized/sage/rings/complex_double.c:6109)
        return ComplexDoubleElement(x, 0)
      File "sage/rings/complex_double.pyx", line 755, in sage.rings.complex_double.ComplexDoubleElement.__init__ (build/cythonized/sage/rings/complex_double.c:9194)
        self._complex = gsl_complex_rect(real, imag)
      File "cypari2/gen.pyx", line 2002, in cypari2.gen.Gen.__float__
      File "cypari2/handle_error.pyx", line 213, in cypari2.handle_error._pari_err_handle
    cypari2.handle_error.PariError: incorrect type in gtodouble [t_REAL expected] (t_COMPLEX)

---

New commits:

2cc2fa7	src/sage/rings/real_double.pyx: Replace cimport from cypari.convert by method-local import
d641dc6	src/sage/rings/complex_double.pyx: Move PARI conversion functions to src/sage/libs/pari/

[mkoeppe]

Commit: d641dc6

[kliem]

Changed branch from u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 to u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2

[kliem]

Changed commit from d641dc6 to bc1710c

[kliem]

comment:4

Checking the most obvious thing helped:

sage: from sage.rings.complex_double import pari_gen
sage: pari_gen
()

---

New commits:

e43dfad	Merge branch 'u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2' of git://trac.sagemath.org/sage into u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2
bc1710c	fix incorrect import

[kliem]

Changed author from Matthias Koeppe, ... to Matthias Koeppe, Jonathan Kliem

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

71b62be	new_gen_from_double not available as python function
64984be	coverage

[sagetrac-git]

Changed commit from bc1710c to 64984be

[mkoeppe]

comment:7

Thank you! Let's wait for the patchbot

[mkoeppe]

comment:8

Testing locally, I am seeing:

[sagelib-9.5.beta4] warning: sage/libs/pari/convert_sage_complex_double.pxd:4:44: Declarations should not be declared inline.
[sagelib-9.5.beta4] warning: sage/libs/pari/convert_sage_complex_double.pyx:9:6: Function 'pari_to_cdf' previously declared as 'cpdef'

[mkoeppe]

Changed branch from u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 to u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2

[mkoeppe]

Reviewer: ..., Matthias Koeppe

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

16d2850

src/sage/libs/pari/convert_sage_complex_double.pxd (pari_to_cdf): cdef inline -> cpdef

[sagetrac-git]

Changed commit from 64984be to 16d2850

[tscrim]

comment:13

I think this requires some checking for speed regressions because the function is both no longer inline nor cimport-ed, and I could see this being used in a tight loop.

[kliem]

comment:14

Replying to @tscrim:

I think this requires some checking for speed regressions because the function is both no longer inline nor cimport-ed, and I could see this being used in a tight loop.

Things get about 1µs slower:

Before:

sage: a = CDF(1,2)
sage: %timeit a.__pari__()
199 ns ± 0.214 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit CDF(p)
310 ns ± 0.827 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: b = RDF(1.3)
sage: %timeit b.__pari__()
160 ns ± 0.128 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: p = b.__pari__()
sage: %timeit RDF(p)
174 ns ± 2.06 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.eta()
7.99 µs ± 12.5 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) 
sage: %timeit a.dilog()
46.2 µs ± 64.8 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
sage: %timeit a.gamma()
33.5 µs ± 59.2 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
sage: %timeit a.zeta()
74.4 µs ± 726 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

After:

sage: a = CDF(1,2)
sage: %timeit a.__pari__()
736 ns ± 0.283 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit CDF(p)
327 ns ± 0.544 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: b = RDF(1.3)
sage: %timeit b.__pari__()
707 ns ± 0.792 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = b.__pari__()
sage: %timeit RDF(p)
157 ns ± 0.072 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.eta()
9.31 µs ± 32.7 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
sage: %timeit a.dilog()
48.1 µs ± 389 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
sage: %timeit a.gamma()
36.8 µs ± 549 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
sage: %timeit a.zeta()
76.2 µs ± 1.62 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

I doubt that calling __pari__ or _element_constructor_ is the crucial task of some algorithm.

If the regression is a problem, we could find a faster way to deal with the whole pari_to_cdf(self.__pari__()...) calls, e.g.

diff --git a/src/sage/libs/pari/convert_sage_complex_double.pyx b/src/sage/libs/pari/convert_sage_complex_double.pyx
index 5a04aff7f6..f8859a7dea 100644
--- a/src/sage/libs/pari/convert_sage_complex_double.pyx
+++ b/src/sage/libs/pari/convert_sage_complex_double.pyx
@@ -58,3 +58,6 @@ cpdef Gen new_gen_from_complex_double_element(ComplexDoubleElement self):
         return new_gen_from_double(self._complex.real)
     else:
         return new_t_COMPLEX_from_double(self._complex.real, self._complex.imag)
+
+cpdef ComplexDoubleElement complex_double_eta(ComplexDoubleElement self, int flag):
+    return pari_to_cdf(new_gen_from_complex_double_element(self).eta(flag))
diff --git a/src/sage/rings/complex_double.pyx b/src/sage/rings/complex_double.pyx
index ecc326b439..0aef6a3aad 100644
--- a/src/sage/rings/complex_double.pyx
+++ b/src/sage/rings/complex_double.pyx
@@ -92,9 +92,11 @@ from sage.structure.coerce cimport is_numpy_type
 try:
     from cypari2.gen import Gen as pari_gen
     from sage.libs.pari.convert_sage_complex_double import pari_to_cdf
+    from sage.libs.pari.convert_sage_complex_double import complex_double_eta
 
 except ImportError:
     pari_gen = ()
+    complex_double_eta = None
 
 from . import complex_mpfr
 
@@ -2241,8 +2243,11 @@ cdef class ComplexDoubleElement(FieldElement):
             # this, PARI can easily underflow.
             return ComplexDoubleElement(0,0)
 
+        if complex_double_eta is None:
+            raise ImportError("pari not found")
+
         cdef int flag = 0 if omit_frac else 1
-        return pari_to_cdf(self.__pari__().eta(flag))
+        return complex_double_eta(self, flag)

This is even faster than the original approach with new timing being:

sage: a = CDF(1,2)
sage: %timeit a.eta()
7.89 µs ± 9.65 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

This approach could be used anywhere but to speed up __pari__ and _element_constructor_.

[tscrim]

comment:15

Thank you for doing the timings. If someone is using polynomials or matrices over RDF/CDF and then running a PARI algorithm on them and has to convert back and forth, then this could have some noticeable effects on speed. With this, that main thing it would be going through is __pari__ and _element_constructor_. Although my suspicion is that this would not have a major impact, if someone is doing that.

Perhaps this is just the cost of doing business, but I would have expected this to be okay to do. To be precise about "this", I mean implementing a Cython conversion to/from a library. How useful is this to modularization?

I am actually more worried about #30022 having an actual impact on calculations because ZZ and QQ are much more common for base ring computations.

[kliem]

comment:16

If you really want to be fast, you should with __pari__ and _element_constructor_ you can use pari_to_cdf or new_gen_from_complex_double_element directly:

sage: from sage.libs.pari.convert_sage_complex_double import pari_to_cdf, new_gen_from_complex_double_element
sage: a = CDF(1,2)
sage: %timeit new_gen_from_complex_double_element(a)
182 ns ± 0.109 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: p = a.__pari__()
sage: %timeit pari_to_cdf(p)
106 ns ± 0.0893 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In that way, this ticket is an improvement. I'm fixing the other use cases and I will also see, whether #30022 can be improved.

[kliem]

Changed branch from u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 to u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2

[kliem]

New commits:

388b05f	speed up conversion for complex double
a64d72b	speed up conversion from real double

[kliem]

Changed commit from 16d2850 to a64d72b

[kliem]

comment:18

Before this ticket:

sage: a = RDF(2.3)
sage: %timeit a.__pari__()
174 ns ± 0.0989 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: p = a.__pari__()
sage: %timeit RDF(p)
161 ns ± 0.0978 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: a = CDF(1,1)
sage: %timeit a.__pari__()
205 ns ± 0.299 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit CDF(p)
323 ns ± 0.852 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.eta()
7.38 µs ± 18.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
sage: %timeit a.gamma()
33.2 µs ± 74.6 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

With this ticket:

sage: a = RDF(2.3)
sage: %timeit a.__pari__()
173 ns ± 0.137 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: p = a.__pari__()
sage: %timeit RDF(p)
170 ns ± 0.109 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: a = CDF(1,1)
sage: %timeit a.__pari__()
214 ns ± 0.392 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit CDF(p)
376 ns ± 0.567 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.eta()
7.41 µs ± 18.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
sage: %timeit a.gamma()
33.2 µs ± 42.6 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

[tscrim]

comment:19

That requires more understanding of the internal workings of Sage, is not local to the parent object, and is not flexible when the parent changes. So I am not sure it is the best path.

However, at least with these changes, the effect on the function calls are minimized.

[mkoeppe]

comment:22

Replying to @tscrim:

How useful is this to modularization?

Not sure if I'm answering this in the right context of your question, but I have expanded the ticket description to explain the motivation for this ticket.

[tscrim]

Changed reviewer from ..., Matthias Koeppe to Matthias Koeppe, Travis Scrimshaw

[tscrim]

comment:23

That was what I was after. Thank you.

The current version LGTM.

[mkoeppe]

comment:24

Thanks for reviewing!

[mkoeppe]

Changed branch from u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 to u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2

[mkoeppe]

Changed commit from a64d72b to addbbe7

[mkoeppe]

New commits:

addbbe7

Merge tag '9.5.beta5' into t/32701/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2

[mkoeppe]

comment:27

I used an old version of the branch, to be redone

[mkoeppe]

Changed branch from u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 to u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2

[mkoeppe]

Changed commit from addbbe7 to a64d72b

[mkoeppe]

New commits:

2cc2fa7	src/sage/rings/real_double.pyx: Replace cimport from cypari.convert by method-local import
d641dc6	src/sage/rings/complex_double.pyx: Move PARI conversion functions to src/sage/libs/pari/
e43dfad	Merge branch 'u/mkoeppe/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2' of git://trac.sagemath.org/sage into u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2
bc1710c	fix incorrect import
71b62be	new_gen_from_double not available as python function
64984be	coverage
16d2850	src/sage/libs/pari/convert_sage_complex_double.pxd (pari_to_cdf): cdef inline -> cpdef
388b05f	speed up conversion for complex double
a64d72b	speed up conversion from real double

[mkoeppe]

comment:29

Merging 9.5.beta5 pulls in #32607, so corresponding changes need to be made in src/sage/libs/pari/convert_sage_complex_double.pyx

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

629c319

merge in 9.5.beta5

[sagetrac-git]

Changed commit from a64d72b to 629c319

[mkoeppe]

comment:32

Thanks.

[vbraun]

Changed branch from u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 to 629c319