sage.rings.real_double: Move methods using GSL to a separate extension module

Question

sage.rings.real_double: Move methods using GSL to a separate extension module

Closed this issue 4 years ago · 44 comments

Discussion: https://groups.google.com/g/sage-devel/c/HTN6KoqZVJQ/m/2BzAx5erBAAJ

We first replace use of some gsl functions like gsl_isnan by their C standard library counterparts, as provided by Cython's libc.math (
https://github.com/cython/cython/blob/master/Cython/Includes/libc/math.pxd)

Likewise for some uses of these functions in src/sage/plot/plot3d/implicit_surface.pyx

We move the remaining methods that use gsl functions to a new subclass RealDoubleElement_gsl, in the new module sage.rings.real_double_element_gsl.

RealDoubleElement already uses a nonstandard allocator for speed. We adjust it so that it actually creates an instance of RealDoubleElement_gsl when the module sage.rings.real_double_element_gsl can be imported.

(Making similar changes to ComplexDoubleElement is beyond the scope of this ticket. This is more complicated because gsl_complex may be a type separate from the C99 double complex type, see https://www.gnu.org/software/gsl/doc/html/complex.html)

Depends on #32682
Depends on #32675

CC: @dimpase @kiwifb @williamstein @tscrim

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 520da8f

Reviewer: François Bissey

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

Answer 1 · 2021-10-12T23:03:10.000Z

Description changed:

--- 
+++ 
@@ -1,3 +1,4 @@
 Discussion: https://groups.google.com/g/sage-devel/c/HTN6KoqZVJQ/m/2BzAx5erBAAJ
 
-
+We first replace use of gsl functions like `gsl_isnan` by their C standard library counterparts, as provided by Cython's `libc.math` (
+https://github.com/cython/cython/blob/master/Cython/Includes/libc/math.pxd)

Answer 2 · 2021-10-12T23:13:10.000Z

Branch: u/mkoeppe/sage_rings_real_double__move_methods_using_gsl_to_a_separate_extension_module

Answer 3 · 2021-10-12T23:13:35.000Z

Commit: f04ad5a

Answer 4 · 2021-10-12T23:13:35.000Z

comment:3

This simple change already breaks stuff.... what am I doing wrong?

New commits:

`f04ad5a`	`src/sage/rings/real_double.pyx: Replace gsl_{isnan,isinf,finite} by libc.math functions`

Answer 5 · 2021-10-12T23:27:56.000Z

comment:4

Are you doing an incremental build? I pointed sage-on-gentoo to your commit and so far it has compiled sage for python3.8 and python3.9 is on its way. I won't know about doc building breakages without being a bit more involved.

Answer 6 · 2021-10-12T23:39:49.000Z

comment:5

Installing "sagemath-standard" works here as far as I can tell.

Answer 7 · 2021-10-12T23:46:12.000Z

comment:6

Yes, it compiles but doctests fail:

sage -t --random-seed=0 src/sage/rings/infinity.py  # 4 doctests failed
sage -t --random-seed=0 src/sage/rings/complex_double.pyx  # 4 doctests failed
sage -t --random-seed=0 src/sage/rings/real_double.pyx  # 10 doctests failed

Answer 8 · 2021-10-12T23:51:04.000Z

Description changed:

--- 
+++ 
@@ -1,4 +1,6 @@
 Discussion: https://groups.google.com/g/sage-devel/c/HTN6KoqZVJQ/m/2BzAx5erBAAJ
 
-We first replace use of gsl functions like `gsl_isnan` by their C standard library counterparts, as provided by Cython's `libc.math` (
+We first replace use of some gsl functions like `gsl_isnan` by their C standard library counterparts, as provided by Cython's `libc.math` (
 https://github.com/cython/cython/blob/master/Cython/Includes/libc/math.pxd)
+
+

Answer 9 · 2021-10-13T00:15:46.000Z

comment:9

I only get those

sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/complex_double.pyx  # 2 doctests failed
sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/real_double.pyx  # 2 doctests failed

And it looks like conversion from boolean to integer issues

fbissey@tarazed ~ $ sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/complex_double.pyx
too many failed tests, not using stored timings
Running doctests with ID 2021-10-13-13-12-48-70eba795.
Using --optional=dochtml,pip,sage
Doctesting 1 file.
sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/complex_double.pyx
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/rings/complex_double.pyx", line 1626, in sage.rings.complex_double.ComplexDoubleElement.is_infinity
Failed example:
    CDF(1, 2).is_infinity()
Expected:
    False
Got:
    0
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/rings/complex_double.pyx", line 1628, in sage.rings.complex_double.ComplexDoubleElement.is_infinity
Failed example:
    CDF(0, oo).is_infinity()
Expected:
    True
Got:
    1
**********************************************************************
1 item had failures:
   2 of   3 in sage.rings.complex_double.ComplexDoubleElement.is_infinity
    [345 tests, 2 failures, 0.18 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/complex_double.pyx  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 0.3 seconds
    cpu time: 0.2 seconds
    cumulative wall time: 0.2 seconds
Pytest is not installed, skip checking tests that rely on it.
fbissey@tarazed ~ $ sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/real_double.pyx
too many failed tests, not using stored timings
Running doctests with ID 2021-10-13-13-13-13-b89854b8.
Using --optional=dochtml,pip,sage
Doctesting 1 file.
sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/real_double.pyx
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/rings/real_double.pyx", line 1730, in sage.rings.real_double.RealDoubleElement.is_infinity
Failed example:
    (a/b).is_infinity()
Expected:
    True
Got:
    1
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/rings/real_double.pyx", line 1732, in sage.rings.real_double.RealDoubleElement.is_infinity
Failed example:
    (b/a).is_infinity()
Expected:
    False
Got:
    0
**********************************************************************
1 item had failures:
   2 of   4 in sage.rings.real_double.RealDoubleElement.is_infinity
    [454 tests, 2 failures, 0.15 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/rings/real_double.pyx  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 0.2 seconds
    cpu time: 0.1 seconds
    cumulative wall time: 0.2 seconds

Answer 10 · 2021-10-15T22:06:07.000Z

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

`d3caeb4`	`src/sage/rings/real_double.pyx: Fixup: Replace gsl_{isnan,isinf,finite} by libc.math functions`

Answer 11 · 2021-10-15T22:06:07.000Z

Changed commit from f04ad5a to d3caeb4

Answer 12 · 2021-10-15T22:46:00.000Z

Description changed:

--- 
+++ 
@@ -3,4 +3,6 @@
 We first replace use of some gsl functions like `gsl_isnan` by their C standard library counterparts, as provided by Cython's `libc.math` (
 https://github.com/cython/cython/blob/master/Cython/Includes/libc/math.pxd)
 
+Likewise for some uses of these functions in `src/sage/plot/plot3d/implicit_surface.pyx`
 
+

Answer 13 · 2021-10-16T02:51:53.000Z

Changed commit from d3caeb4 to 31d188f

Answer 14 · 2021-10-16T02:51:53.000Z

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

`e340b2c`	`sage.misc.allocator: Add cdef hook_tp_functions_type`
`31d188f`	`sage.rings.real_double_element_gsl: Split out from sage.rings.real_double`

Answer 15 · 2021-10-16T02:52:20.000Z

Author: Matthias Koeppe

Answer 16 · 2021-10-16T02:55:35.000Z

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

`5c29e6c`	`src/sage/plot/plot3d/implicit_surface.pyx: Use libc.math.isnan instead of gsl_isnan`

Answer 17 · 2021-10-16T02:55:35.000Z

Changed commit from 31d188f to 5c29e6c

Answer 18 · 2021-10-16T02:59:05.000Z

Description changed:

--- 
+++ 
@@ -5,4 +5,7 @@
 
 Likewise for some uses of these functions in `src/sage/plot/plot3d/implicit_surface.pyx`
 
+We move the remaining methods that use gsl functions to a new subclass `RealDoubleElement_gsl`, in the new module `sage.rings.real_double_element_gsl`.
 
+`RealDoubleElement` already uses a nonstandard allocator for speed. We adjust it so that it actually creates an instance of `RealDoubleElement_gsl` when the module `sage.rings.real_double_element_gsl` can be imported.
+

Answer 19 · 2021-10-16T05:38:30.000Z

Description changed:

--- 
+++ 
@@ -9,3 +9,5 @@
 
 `RealDoubleElement` already uses a nonstandard allocator for speed. We adjust it so that it actually creates an instance of `RealDoubleElement_gsl` when the module `sage.rings.real_double_element_gsl` can be imported.
 
+(Making similar changes to `ComplexDoubleElement` is beyond the scope of this ticket.  This is more complicated because `gsl_complex` may be a type separate from the C99 `double complex` type.)
+

Answer 20 · 2021-10-16T05:38:49.000Z

Description changed:

--- 
+++ 
@@ -9,5 +9,5 @@
 
 `RealDoubleElement` already uses a nonstandard allocator for speed. We adjust it so that it actually creates an instance of `RealDoubleElement_gsl` when the module `sage.rings.real_double_element_gsl` can be imported.
 
-(Making similar changes to `ComplexDoubleElement` is beyond the scope of this ticket.  This is more complicated because `gsl_complex` may be a type separate from the C99 `double complex` type.)
+(Making similar changes to `ComplexDoubleElement` is beyond the scope of this ticket.  This is more complicated because `gsl_complex` may be a type separate from the C99 `double complex` type, see https://www.gnu.org/software/gsl/doc/html/complex.html)

Answer 21 · 2021-10-18T04:49:40.000Z

comment:20

@kiwifb Are you going to do a full review of this?

From the patchbot, we have three trivial failures:

sage -t --long --random-seed=0 src/sage/misc/functional.py
**********************************************************************
File "src/sage/misc/functional.py", line 1540, in sage.misc.functional.round
Failed example:
    type(q)
Expected:
    <type 'sage.rings.real_double.RealDoubleElement'>
Got:
    <class 'sage.rings.real_double_element_gsl.RealDoubleElement_gsl'>
**********************************************************************

sage -t --long --random-seed=0 src/sage/libs/mpmath/utils.pyx
**********************************************************************
File "src/sage/libs/mpmath/utils.pyx", line 408, in sage.libs.mpmath.utils.call
Failed example:
    type(_)
Expected:
    <type 'sage.rings.real_double.RealDoubleElement'>
Got:
    <class 'sage.rings.real_double_element_gsl.RealDoubleElement_gsl'>
**********************************************************************

sage -t --long --random-seed=0 src/sage/symbolic/expression.pyx
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 1689, in sage.symbolic.expression.Expression._convert
Failed example:
    type(_.pyobject())
Expected:
    <type 'sage.rings.real_double.RealDoubleElement'>
Got:
    <class 'sage.rings.real_double_element_gsl.RealDoubleElement_gsl'>
**********************************************************************

Answer 22 · 2021-10-18T05:14:24.000Z

Dependencies: #32682, #32675

Answer 23 · 2021-10-18T05:17:19.000Z

Changed commit from 5c29e6c to 3d5bad2

Answer 24 · 2021-10-18T05:17:19.000Z

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

`a6c9428`	`change`
`f3f2919`	`Merge #32682`
`46cf067`	`convert some`
`471b52f`	`fix wrong change`
`b3d941b`	`Merge #32675`
`3d5bad2`	`Make doctests accept RealDoubleElement and RealDoubleElement_gsl`

Answer 25 · 2021-10-18T06:02:56.000Z

comment:23

Was about to do a bit of reviewing. I need to pull the latest changes now.

Answer 26 · 2021-10-18T06:36:55.000Z

comment:24

I will leave the review to you :).

Answer 27 · 2021-10-18T07:56:25.000Z

Reviewer: François Bissey

Answer 28 · 2021-10-18T07:56:25.000Z

comment:25

Let's move this on.

Answer 29 · 2021-10-18T16:10:09.000Z

comment:26

Thanks!

Answer 30 · 2021-10-20T23:00:57.000Z

Changed branch from u/mkoeppe/sage_rings_real_double__move_methods_using_gsl_to_a_separate_extension_module to 3d5bad2

Answer 31 · 2021-10-21T22:40:57.000Z

comment:28

Lots of failures of the form

sage -t --long --random-seed=95492849111503266761374143413906402145 src/doc/de/thematische_anleitungen/sage_gymnasium.rst
**********************************************************************
File "src/doc/de/thematische_anleitungen/sage_gymnasium.rst", line 667, in doc.de.thematische_anleitungen.sage_gymnasium
Failed example:
    plot(f, xmin=-2, xmax=2, ymin=-10, ymax = 10, detect_poles=True)
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest doc.de.thematische_anleitungen.sage_gymnasium[1]>", line 1, in <module>
        plot(f, xmin=-Integer(2), xmax=Integer(2), ymin=-Integer(10), ymax = Integer(10), detect_poles=True)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/misc/decorators.py", line 491, in wrapper
        return func(*args, **options)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/plot/plot.py", line 1957, in plot
        G = funcs.plot(*args, **original_opts)
      File "sage/symbolic/expression.pyx", line 12775, in sage.symbolic.expression.Expression.plot (build/cythonized/sage/symbolic/expression.cpp:94231)
        return plot(f, *args, **kwds)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/misc/decorators.py", line 491, in wrapper
        return func(*args, **options)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/plot/plot.py", line 1972, in plot
        G = _plot(funcs, (xmin, xmax), **kwds)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/plot/plot.py", line 2419, in _plot
        alpha = (RDF(dy)/RDF(dx)).arctan()
      File "sage/structure/element.pyx", line 494, in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4755)
        return self.getattr_from_category(name)
      File "sage/structure/element.pyx", line 507, in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4867)
        return getattr_from_other_class(self, cls, name)
      File "sage/cpython/getattr.pyx", line 363, in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2634)
        raise AttributeError(dummy_error_message)
    AttributeError: 'sage.rings.real_double.RealDoubleElement' object has no attribute 'arctan'
**********************************************************************
File "src/doc/de/thematische_anleitungen/sage_gymnasium.rst", line 674, in doc.de.thematische_anleitungen.sage_gymnasium
Failed example:
    plot(f, xmin=-2, xmax=2, ymin=-10, ymax = 10, detect_poles="show")
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest doc.de.thematische_anleitungen.sage_gymnasium[1]>", line 1, in <module>
        plot(f, xmin=-Integer(2), xmax=Integer(2), ymin=-Integer(10), ymax = Integer(10), detect_poles="show")
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/misc/decorators.py", line 491, in wrapper
        return func(*args, **options)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/plot/plot.py", line 1957, in plot
        G = funcs.plot(*args, **original_opts)
      File "sage/symbolic/expression.pyx", line 12775, in sage.symbolic.expression.Expression.plot (build/cythonized/sage/symbolic/expression.cpp:94231)
        return plot(f, *args, **kwds)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/misc/decorators.py", line 491, in wrapper
        return func(*args, **options)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/plot/plot.py", line 1972, in plot
        G = _plot(funcs, (xmin, xmax), **kwds)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.9/lib64/python3.9/site-packages/sage/plot/plot.py", line 2419, in _plot
        alpha = (RDF(dy)/RDF(dx)).arctan()
      File "sage/structure/element.pyx", line 494, in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4755)
        return self.getattr_from_category(name)
      File "sage/structure/element.pyx", line 507, in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4867)
        return getattr_from_other_class(self, cls, name)
      File "sage/cpython/getattr.pyx", line 363, in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2634)
        raise AttributeError(dummy_error_message)
    AttributeError: 'sage.rings.real_double.RealDoubleElement' object has no attribute 'arctan'
**********************************************************************
1 item had failures:
   2 of 294 in doc.de.thematische_anleitungen.sage_gymnasium
    [207 tests, 2 failures, 2.47 s]
----------------------------------------------------------------------
sage -t --long --random-seed=95492849111503266761374143413906402145 src/doc/de/thematische_anleitungen/sage_gymnasium.rst  # 2 doctests failed
----------------------------------------------------------------------

New commits:

`f04ad5a`	`src/sage/rings/real_double.pyx: Replace gsl_{isnan,isinf,finite} by libc.math functions`
`d3caeb4`	`src/sage/rings/real_double.pyx: Fixup: Replace gsl_{isnan,isinf,finite} by libc.math functions`
`e340b2c`	`sage.misc.allocator: Add cdef hook_tp_functions_type`
`31d188f`	`sage.rings.real_double_element_gsl: Split out from sage.rings.real_double`
`5c29e6c`	`src/sage/plot/plot3d/implicit_surface.pyx: Use libc.math.isnan instead of gsl_isnan`
`f3f2919`	`Merge #32682`
`b3d941b`	`Merge #32675`
`3d5bad2`	`Make doctests accept RealDoubleElement and RealDoubleElement_gsl`

Answer 32 · 2021-10-21T22:40:57.000Z

Changed branch from 3d5bad2 to u/mkoeppe/sage_rings_real_double__move_methods_using_gsl_to_a_separate_extension_module

Answer 33 · 2021-10-21T23:10:07.000Z

comment:29

With some diagnostics added:

diff --git a/src/sage/rings/real_double.pyx b/src/sage/rings/real_double.pyx
index 79ddbcf279..03f1ea767e 100644
--- a/src/sage/rings/real_double.pyx
+++ b/src/sage/rings/real_double.pyx
@@ -2178,9 +2178,11 @@ from sage.misc.allocator cimport hook_tp_functions, hook_tp_functions_type
 hook_tp_functions(global_dummy_element, <newfunc>(&fast_tp_new), <destructor>(&fast_tp_dealloc), False)
 try:
     from .real_double_element_gsl import RealDoubleElement_gsl
-except Exception:
+except Exception as e:
+    print("Error: {}".format(e))
     pass
 else:
+    print("Hooking RealDoubleElement_gsl")
     # global_dummy_element is of type RealDoubleElement_gsl,
     # so hook the base class now.
     hook_tp_functions_type(RealDoubleElement, <newfunc>(&fast_tp_new), <destructor>(&fast_tp_dealloc), False)

one sees: Error: cannot import name ComplexField.

Answer 34 · 2021-10-21T23:13:18.000Z

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

`6fcaa9b`	`Merge tag '9.5.beta4' into t/32677/sage_rings_real_double__move_methods_using_gsl_to_a_separate_extension_module`
`78de558`	`src/sage/rings/real_double_element_gsl.pyx: Remove module-level import of sage.rings.complex_double to remove cyclic imports`

Answer 35 · 2021-10-21T23:13:18.000Z

Changed commit from 3d5bad2 to 78de558

Answer 36 · 2021-10-22T00:32:09.000Z

comment:32

I hadn't seen any of that in a full ptestlong, why did Volker see something and not me?

Answer 37 · 2021-10-22T00:37:53.000Z

comment:33

This failure came from the combination with other changes that 9.5.beta4 has included, which must have made some changes to the import order.

Answer 38 · 2021-10-22T00:39:31.000Z

comment:34

OK, that would make sense. I am having a long week end, if someone want to do a new review before Monday evening (NZT) they are welcome to it.

Answer 39 · 2021-10-26T08:32:11.000Z

comment:35

Back to positive review for me.

Answer 40 · 2021-10-26T18:45:52.000Z

comment:36

Thank you!

Answer 41 · 2021-10-28T23:47:07.000Z

Changed commit from 78de558 to 520da8f

Answer 42 · 2021-10-28T23:47:07.000Z

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

`520da8f`	`Merge tag '9.5.beta5' into t/32677/sage_rings_real_double__move_methods_using_gsl_to_a_separate_extension_module`

Answer 43 · 2021-10-28T23:47:28.000Z

comment:38

trivial merge

Answer 44 · 2021-10-31T22:21:10.000Z

Changed branch from u/mkoeppe/sage_rings_real_double__move_methods_using_gsl_to_a_separate_extension_module to 520da8f