sage.rings.{real,complex}_double: Remove compile time dependency on cypari2
Closed this issue · 44 comments
... as previously done in #30022 for sage.rings.integer and sage.rings.rational.
This is to support modularization (#29705):
- in particular, for sagemath-categories (#29865), to make the functions
py_scalar_parentandpy_scalar_to_element(fromsage.structure.coerce) fully functional without having to pull in PARI (at compile time or run time) - likewise, for 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
Author: Matthias Koeppe, ...
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/ |
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 |
Changed author from Matthias Koeppe, ... to Matthias Koeppe, Jonathan Kliem
Thank you! Let's wait for the patchbot
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'
Reviewer: ..., Matthias Koeppe
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 |
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.
Replying to @tscrim:
I think this requires some checking for speed regressions because the function is both no longer
inlinenorcimport-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_.
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.
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.
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)
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.
Yes, having things fast out of the box without extra tricks is important.
Description changed:
---
+++
@@ -1 +1,6 @@
-... like done in #30022
+... as previously done in #30022 for `sage.rings.integer` and `sage.rings.rational`.
+
+This is to support modularization (#29705):
+- in particular, for **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** (#32432) to support floating-point polyhedra and linear programming without having to pull in PARI (at compile time or run time).
+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.
Description changed:
---
+++
@@ -1,6 +1,6 @@
... as previously done in #30022 for `sage.rings.integer` and `sage.rings.rational`.
This is to support modularization (#29705):
-- in particular, for **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)
+- in particular, for **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** (#32432) to support floating-point polyhedra and linear programming without having to pull in PARI (at compile time or run time).
Changed reviewer from ..., Matthias Koeppe to Matthias Koeppe, Travis Scrimshaw
That was what I was after. Thank you.
The current version LGTM.
Thanks for reviewing!
New commits:
addbbe7 | Merge tag '9.5.beta5' into t/32701/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 |
I used an old version of the branch, to be redone
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 |
Merging 9.5.beta5 pulls in #32607, so corresponding changes need to be made in src/sage/libs/pari/convert_sage_complex_double.pyx
Branch pushed to git repo; I updated commit sha1. New commits:
629c319 | merge in 9.5.beta5 |
Thanks.
Changed branch from u/gh-kliem/sage_rings__real_complex__double__remove_compile_time_dependency_on_cypari2 to 629c319