Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatibilities in Sage mpmath cause downstream issues in SymPy #25445

Open
asmeurer opened this issue May 25, 2018 · 17 comments · Fixed by passagemath/passagemath#22 · May be fixed by #38113
Open

Incompatibilities in Sage mpmath cause downstream issues in SymPy #25445

asmeurer opened this issue May 25, 2018 · 17 comments · Fixed by passagemath/passagemath#22 · May be fixed by #38113

Comments

@asmeurer
Copy link

The Sage mpmath has some incompatibilities with non-sage mpmath, which causes some issues in SymPy, since mpmath always uses the Sage backend if Sage is installed.

You can get a full idea of the issues by running the SymPy tests in Sage (import sympy;sympy.test()). A few failures are also due to some gmpy2 issues (gmpy2 types fail on Sage integers).

The issues I've noticed so far are:

  • The sage mpmath types do not subclass from mpmath.ctx_mp.mpnumeric, which is the base class of mpf and mpc according to its own docstring.

  • The sage mpf type does not have a context attribute.

Another issue, reported in sympy/sympy#19690:

import sympy.physics
from sympy.physics.units import meter
import copy
copy.deepcopy(3.7*meter)
[...]
File "sage/rings/integer.pyx", line 7143, in sage.rings.integer.mpz_set_str_python (build/cythonized/sage/rings/inte
ger.c:43554)
TypeError: unable to convert '0xecccccccccccd' to an integer

(fixed in sympy/sympy#21996, merged for inclusion in Sympy 1.9)

References:

CC: @antonio-rojas @kiwifb @fredrik-johansson @isuruf @nbruin @saraedum @slel @soehms @strogdon @videlec

Component: distribution

Keywords: mpmath

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

@asmeurer asmeurer added this to the sage-8.3 milestone May 25, 2018
@slel
Copy link
Member

slel commented Mar 18, 2021

Changed keywords from none to mpmath

@slel

This comment has been minimized.

@slel slel modified the milestones: sage-8.3, sage-9.4 Mar 18, 2021
@kiwifb
Copy link
Member

kiwifb commented Mar 18, 2021

comment:2

The content of the description echoes some of what I said on sage-devel, we are probably in this situation because sage misuses mpmath, or uses some parts that should be private.

@soehms

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Mar 26, 2021

comment:7

mpmath update on #31564

@mkoeppe
Copy link
Contributor

mkoeppe commented May 10, 2021

comment:8

Moving to 9.4, as 9.3 has been released.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 May 10, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Oct 30, 2021

comment:13

See also #32799: Explicitly set MPMATH_SAGE env variable.

@sheerluck
Copy link
Contributor

Replying to @asmeurer:

(fixed in sympy/sympy#21996, merged for inclusion in Sympy 1.9)

Yes, commit 9737261 is in master branch, but not in 1.9 branch and not in released sympy-1.9.tar.gz so deepcopy(3.7*meter) not gonna work :(

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@sheerluck
Copy link
Contributor

comment:16

Speaking about import sympy;sympy.test(), there are 75 failed tests, most of them
failed due to an incomplete int_types in mpmath:

$ ipython

In [1]: import mpmath.libmp.backend as mb

In [2]: mb.int_types
Out[2]: (int, mpz)

and

$ sage -q
sage: import mpmath.libmp.backend as mb
sage: mb.int_types
(<class 'int'>, <class 'sage.rings.integer.Integer'>)

But when mpmath is patched this way:

--- a/mpmath/libmp/backend.py
+++ b/mpmath/libmp/backend.py
@@ -76,6 +76,7 @@
         if gmpy.version() >= '1.03':
             BACKEND = 'gmpy'
             MPZ = gmpy.mpz
+            GMPY_MPZ = gmpy.mpz
     except:
         pass
 
@@ -111,5 +112,7 @@
 except NameError:
     if BACKEND == 'python':
         int_types = (int,)
+    elif BACKEND == 'sage':
+        int_types = (int, GMPY_MPZ, MPZ_TYPE)
     else:
         int_types = (int, MPZ_TYPE)

we have

$ sage -q
sage: import mpmath.libmp.backend as mb
sage: mb.int_types
(<class 'int'>, <class 'mpz'>, <class 'sage.rings.integer.Integer'>)

and number of failed tests went from 75 to 30.

When sympy is patched this way:

--- a/sympy/core/numbers.py
+++ b/sympy/core/numbers.py
@@ -4469,6 +4469,14 @@
 _sympy_converter[mpnumeric] = sympify_mpmath


+def sympify_sage(x):
+    from mpmath import mp
+    from sage.libs.mpmath.utils import sage_to_mpmath
+    return Expr._from_mpmath(sage_to_mpmath(x, mp.prec), mp.prec)
+
+_sympy_converter["sage"] = sympify_sage
+
+
 def sympify_complex(a):
     real, imag = list(map(sympify, (a.real, a.imag)))
     return real + S.ImaginaryUnit*imag
--- a/sympy/core/sympify.py
+++ b/sympy/core/sympify.py
@@ -382,6 +382,11 @@
         if conv is not None:
             return conv(a)

+    if hasattr(a, "_mpf_") or hasattr(a, "_mpc_"):
+        conv = _sympy_converter.get("sage")
+        if conv is not None:
+            return conv(a)
+
     if cls is type(None):
         if strict:
             raise SympifyError(a)
--- a/sympy/solvers/solvers.py
+++ b/sympy/solvers/solvers.py
@@ -2919,7 +2919,8 @@
                 f[i] = fi.lhs - fi.rhs
         f = Matrix(f).T
     if iterable(x0):
-        x0 = list(x0)
+        if not hasattr(x0, "imag"):
+            x0 = list(x0)
     if not isinstance(f, Matrix):
         # assume it's a SymPy expression
         if isinstance(f, Eq):

number of failed tests went from 30 to 13.

We can fix "TypeError: '>=' not supported between instances of 'mpz' and
'sage.libs.mpmath.ext_main.mpf'" with something like this

--- a/sage/libs/mpmath/ext_main.pyx
+++ b/sage/libs/mpmath/ext_main.pyx
@@ -46,6 +46,8 @@
 import mpmath.function_docs as function_docs
 from mpmath.libmp import to_str
 from mpmath.libmp import repr_dps, prec_to_dps, dps_to_prec
+from mpmath.libmp import int_types, mpf_lt, mpf_gt, mpf_le, mpf_ge, mpf_eq
+from mpmath.libmp.libmpf import from_int

 DEF OP_ADD = 0
 DEF OP_SUB = 1
@@ -2101,6 +2103,13 @@
         MPF_normalize(&r.value, global_opts)
         return r

+    def __lt__(s, t): return s._cmp(t, mpf_lt, OP_LT)
+    def __gt__(s, t): return s._cmp(t, mpf_gt, OP_GT)
+    def __le__(s, t): return s._cmp(t, mpf_le, OP_LE)
+    def __ge__(s, t): return s._cmp(t, mpf_ge, OP_GE)
+    def __ne__(s, t): return not s.__eq__(t)
+    def __eq__(s, t): return s._cmp(t, mpf_eq, OP_EQ)
+
     def __abs__(s):
         """
         Computes the absolute value, rounded to the current
@@ -2128,7 +2137,7 @@
         MPF_sqrt(&r.value, &s.value, global_opts)
         return r

-    def __richcmp__(self, other, int op):
+    def _cmp(s, t, func, int op):
         """
         Compares numbers ::

@@ -2139,8 +2148,15 @@
             True
             sage: mpf(3) == 4
             False
+            sage: from gmpy2 import fac
+            sage: mpf(120) == fac(5)
+            True
+            sage: fac(5) >= mpf(3)
+            True
         """
-        return binop(OP_RICHCMP+op, self, other, global_opts)
+        if isinstance(t, int_types):
+            return func(s._mpf_, from_int(t))
+        return binop(op, s, t, global_opts)

Sadly that huge patch fixes only one failed test :)

So we left with 12 test, two of them (in test_holonomic.py) can be fixed using
mpmath.almosteq instead of comparing strings, one test in test_pickling.py
is about SymPyDeprecationWarning, one test in test_lambdify.py is just
"TypeError vs NotImplementedError", and all other failures are just
AssertionErrors and they actually work if you just try it:

sage: import mpmath
sage: from mpmath import mpc
sage: assert mpmath.besselj(2, 1 + 1j).ae(mpc("0.04157988694396212", "0.24739764151330632"))
sage: from sympy.functions.elementary.exponential import exp
sage: assert exp(pi * sqrt(163)).evalf(50).num.ae(262537412640768744)
sage: import sympy
....: import mpmath
....: from sympy.utilities.lambdify import lambdify
....: 
....: f_ = lambdify([x], sympy.LambertW(x, -1), modules="scipy")
sage: assert f_(0.1) == mpmath.lambertw(0.1, -1)
sage: from sympy.functions.special.gamma_functions import uppergamma, lowergamma
sage: expr1 = lambdify((x, y), uppergamma(x, y), "mpmath")(1, 2)
sage: assert expr1 == uppergamma(1, 2).evalf()
sage: from sympy.core.symbol import symbols
sage: from sympy.solvers import nsolve
sage: x, y = symbols("x y")
sage: nsolve(x**2 + 2, 1j)
-5.12588859584049e-25 + 1.4142135623731*I

The only test I can do nothing about is test_P25 in test_wester.py
Oh, and also that 3.7*meter too:

sage: import sympy.physics
....: from sympy.physics.units import meter
....: import copy 
....:
....: copy.deepcopy(3.7 * meter)
---------------------------------------------------------------------------
NotImplementedError: conversion to SageMath is not implemented

All References in this ticket are already fixed even without those three patches earlier in this comment. And this ticket would have been closed as solved if it wasn't for the test_P25 (and 3.7*meter)

@mkoeppe mkoeppe removed this from the sage-9.6 milestone May 3, 2022
@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 21, 2023

Can we solve this problem by vendoring a copy of mpmath for the internal uses within Sage?

@jhpalmieri
Copy link
Member

Could this be the cause of the problem reported at https://ask.sagemath.org/question/73706/sages-python-not-giving-the-expected-result/ ?

@skirpichev
Copy link

Also, cython implementation of mpmath types in sage seems to be outdated (there are test failures in the mpmath test suite for 1.3.0): #36447 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment