Skip to content

Conversation

@mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Aug 26, 2024

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 26, 2024

CI shows numerous failures as reported and analyzed by @kiwifb in #38113

@kiwifb
Copy link
Member

kiwifb commented Aug 26, 2024

I did play with pdb yesterday. The code path in mpmath for sage integer inside mpmath is completely different. In fact the previous code path followed in 1.3.0 does not exist anymore. sage integer do not seem to be recognized as such in 1.4a1.

@kiwifb
Copy link
Member

kiwifb commented Aug 26, 2024

I am wrong, it still exist but sage integer are diverted to a new code path before reaching the right one. In 1.4a1 it seems to take this new path https://github.com/mpmath/mpmath/blob/7afe9ac70c430f7df7e4bc1c904f90bd1887832f/mpmath/ctx_mp_python.py#L82
In 1.3.0 it would take this path
https://github.com/mpmath/mpmath/blob/7afe9ac70c430f7df7e4bc1c904f90bd1887832f/mpmath/ctx_mp_python.py#L86

@kiwifb
Copy link
Member

kiwifb commented Aug 26, 2024

Re-ordering some code in ctx_mp_python.py made my particular example (sage/symbolic/function.pyx) pass. I will run a full doctest with that change now.

@kiwifb
Copy link
Member

kiwifb commented Aug 26, 2024

There is another internal mpmath path that seems to direct sage integers to be treated as rational. I had selected a simple test case to work with and it turns out it was exceptional. But at least I think we have a root cause.

@kiwifb
Copy link
Member

kiwifb commented Aug 26, 2024

The following patch (it is against mpmath master, it may need a tweak for 1.4a1)

diff --git a/mpmath/ctx_mp_python.py b/mpmath/ctx_mp_python.py
index 4f452f2..c61b7d6 100644
--- a/mpmath/ctx_mp_python.py
+++ b/mpmath/ctx_mp_python.py
@@ -79,9 +79,6 @@ def mpf_convert_arg(cls, x, prec, rounding):
         if isinstance(x, int_types): return from_int(x)
         if isinstance(x, float): return from_float(x)
         if isinstance(x, cls.context.constant): return x.func(prec, rounding)
-        if isinstance(x, numbers.Rational): return from_rational(x.numerator,
-                                                                 x.denominator,
-                                                                 prec, rounding)
         if hasattr(x, '_mpf_'): return x._mpf_
         if hasattr(x, '_mpmath_'):
             t = cls.context.convert(x._mpmath_(prec, rounding))
@@ -92,6 +89,9 @@ def mpf_convert_arg(cls, x, prec, rounding):
             if a == b:
                 return a
             raise ValueError("can only create mpf from zero-width interval")
+        if isinstance(x, numbers.Rational): return from_rational(x.numerator,
+                                                                 x.denominator,
+                                                                 prec, rounding)
         if type(x).__module__ == 'decimal':
             return from_Decimal(x, prec, rounding)
         raise TypeError("cannot create mpf from " + repr(x))
@@ -680,6 +680,10 @@ def convert(ctx, x, strings=True):
             return ctx.make_mpc((from_float(x.real), from_float(x.imag)))
         if type(x).__module__ == 'numpy': return ctx.npconvert(x)
         prec, rounding = ctx._prec_rounding
+        if hasattr(x, '_mpf_'): return ctx.make_mpf(x._mpf_)
+        if hasattr(x, '_mpc_'): return ctx.make_mpc(x._mpc_)
+        if hasattr(x, '_mpmath_'):
+            return ctx.convert(x._mpmath_(prec, rounding))
         if isinstance(x, numbers.Rational):
             p, q = x.numerator, x.denominator
             return ctx.make_mpf(from_rational(p, q, prec, rounding))
@@ -689,10 +693,6 @@ def convert(ctx, x, strings=True):
                 return ctx.make_mpf(_mpf_)
             except ValueError:
                 pass
-        if hasattr(x, '_mpf_'): return ctx.make_mpf(x._mpf_)
-        if hasattr(x, '_mpc_'): return ctx.make_mpc(x._mpc_)
-        if hasattr(x, '_mpmath_'):
-            return ctx.convert(x._mpmath_(prec, rounding))
         if type(x).__module__ == 'decimal':
             return ctx.make_mpf(from_Decimal(x, prec, rounding))
         return ctx._convert_fallback(x, strings)

fixes a number of conversion problem. As you can see from the structure of the patch, in current master of mpmath, sage integer are identified as numbers.Rational while they were previously identified as having hasattr(x, '_mpmath_'). The move around helps restore that behavior.

Unfortunately it is not all the problem with mpmath. Now, that I have taken care of this issue another one has gained greater visibility.

sage -t --long --random-seed=158424137200168225311908861487827046848 /usr/lib/python3.12/site-packages/sage/symbolic/constants.py
**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/symbolic/constants.py", line 119, in sage.symbolic.constants
Failed example:
    R(khinchin)
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 716, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.symbolic.constants[28]>", line 1, in <module>
        R(khinchin)
      File "sage/structure/parent.pyx", line 901, in sage.structure.parent.Parent.__call__ (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/structure/parent.c:12762)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 293, in sage.structure.coerce_maps.NamedConvertMap._call_ (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/structure/coerce_maps.c:8515)
        cdef Element e = method(C)
      File "sage/symbolic/expression.pyx", line 1711, in sage.symbolic.expression.Expression._mpfr_ (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/symbolic/expression.cpp:50259)
        return self._eval_self(R)
      File "sage/symbolic/expression.pyx", line 1605, in sage.symbolic.expression.Expression._eval_self (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/symbolic/expression.cpp:49258)
        res = self._convert({'parent':R})
      File "sage/symbolic/expression.pyx", line 1690, in sage.symbolic.expression.Expression._convert (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/symbolic/expression.cpp:49973)
        cdef GEx res = self._gobj.evalf(0, kwds)
      File "sage/symbolic/pynac_impl.pxi", line 2272, in sage.symbolic.expression.py_eval_constant (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/symbolic/expression.cpp:39971)
        return kwds['parent'](constant)
      File "sage/structure/parent.pyx", line 901, in sage.structure.parent.Parent.__call__ (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/structure/parent.c:12762)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 293, in sage.structure.coerce_maps.NamedConvertMap._call_ (/home/portage/sci-mathematics/sagemath-standard-9999/work/sagemath-standard-9999-python3_12/build/cythonized/sage/structure/coerce_maps.c:8515)
        cdef Element e = method(C)
      File "/usr/lib/python3.12/site-packages/sage/symbolic/constants.py", line 1125, in _mpfr_
        return a.eval_constant('khinchin', R)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sage/libs/mpmath/all.py", line 26, in eval_constant
        return ring(_constants_funcs[name](prec)) >> prec
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/mpmath/libmp/libelefun.py", line 91, in g
        return f.memo_val >> (newprec-prec)
               ~~~~~~~~~~~^^~~~~~~~~~~~~~~~
    OverflowError: value could not be converted to C long long
**********************************************************************

again this one appears multiple times and across multiple files.

skirpichev added a commit to skirpichev/mpmath that referenced this pull request Aug 27, 2024
@skirpichev
Copy link

The following patch (it is against mpmath master, it may need a tweak for 1.4a1)

No need to backport this. I'll include sage fixes in next alpha release, see mpmath/mpmath#851

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

The following patch (it is against mpmath master, it may need a tweak for 1.4a1)

No need to backport this. I'll include sage fixes in next alpha release, see mpmath/mpmath#851

Thank you for that. I was going to open an issue on mpmath to discuss it and follow it with a PR if you guys approved the approach. I must say the second issue has me stomped so far. I'll take any clue.

@skirpichev
Copy link

The OverflowError coming from gmpy2. Could you run this test with MPMATH_NOGMPY=1 environment variable set?

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

Yes indeed

fbissey@tarazed ~ $ MPMATH_NOGMPY=1 sage -t --long /usr/lib/python3.12/site-packages/sage/symbolic/constants.pytoo many failed tests, not using stored timings
Running doctests with ID 2024-08-27-20-56-43-57d4cb37.
Running with SAGE_LOCAL='/usr' and SAGE_VENV='/usr'
Using --optional=pip,sage
Features to be detected: 4ti2,SAGE_SRC,benzene,bliss,buckygen,conway_polynomials,coxeter3,csdp,cvxopt,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_ellcurves,database_graphs,database_jones_numfield,database_knotinfo,dvipng,ecm,fpylll,fricas,gap_package_atlasrep,gap_package_design,gap_package_grape,gap_package_guava,gap_package_hap,gap_package_polycyclic,gap_package_qpa,gap_package_quagroup,gfan,glucose,graphviz,imagemagick,ipython,jmol,jupymake,kenzo,kissat,latte_int,lrcalc_python,lrslib,mathics,matroid_database,mcqd,meataxe,mpmath,msolve,nauty,networkx,numpy,palp,pandoc,pdf2svg,pdftocairo,pexpect,phitigra,pillow,plantri,polytopes_db,polytopes_db_4d,pplpy,primecountpy,ptyprocess,pycosat,pycryptosat,pynormaliz,pyparsing,python_igraph,requests,rpy2,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.braiding,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.homfly,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.sat,sage.schemes,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,scipy,singular,sirocco,sphinx,symengine_py,sympy,tdlib,threejs,topcom
Doctesting 1 file.
sage -t --long --random-seed=211690163611789333881822304876232239576 /usr/lib/python3.12/site-packages/sage/symbolic/constants.py
    [241 tests, 0.77 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

I have gmpy2 2.2.1.

@skirpichev
Copy link

Hmm, could you try this workaround for L26 of all.py: ring(_constants_funcs[name](int(prec)))?

I have gmpy2 2.2.1.

Was it installed from wheel or built locally? Is this M$ Windows?

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

Hmm, could you try this workaround for L26 of all.py: ring(_constants_funcs[name](int(prec)))?

I have gmpy2 2.2.1.

Was it installed from wheel or built locally? Is this M$ Windows?

This is Gentoo Linux, my package management builds it locally :)

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

Hmm, could you try this workaround for L26 of all.py: ring(_constants_funcs[name](int(prec)))?

Can you give me a more complete path. There are plenty of all.py in sage.

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

Oh, wait mpmath.

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

Yes, that works.

@skirpichev
Copy link

This is Gentoo Linux, my package management builds it locally :)

Good news, this is something, well, sane... Looks like there is a bug in gmpy2 2.1.x, related with conversion to machine-size integers. Or something is wrong with integration of sage & gmpy2 (__mpz__() method).

@casevh, I would appreciate if you could take a look.

Yes, that works.

ok. Are there other test failures?

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

That fix took care of all the doctest that I am sure were caused by mpmath. I have number of doctest with other identified, or more likely sources. I have just that one left where I am not sure because I have not digged into it yet

sage -t --long --random-seed=158424137200168225311908861487827046848 /usr/lib/python3.12/site-packages/sage/functions/exp_integral.py
**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/functions/exp_integral.py", line 995, in sage.functions.exp_integral.Function_cos_integral._evalf_
Failed example:
    cos_integral(ComplexField(100)(I))                                    # needs sage.symbolic
Expected:
    0.83786694098020824089467857943 + 1.5707963267948966192313216916*I
Got:
    0.83786694098020824089467857944 + 1.5707963267948966192313216916*I
**********************************************************************

but that's just numerical noise.

@skirpichev
Copy link

Well, I see no difference wrt 1.3.0:

>>> from mpmath import *
>>> mp.prec=100
>>> ci(1j)
mpc(real='0.83786694098020824089467857943576', imag='1.5707963267948966192313216916397')
>>> str(_)
'(0.83786694098020824089467857944 + 1.5707963267948966192313216916j)'
>>> import mpmath 
>>> mpmath.__version__
'1.3.0'

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

git blame says that value has been around for 13 years in sage and mpmath 1.3.0 has been in sage since July 2023. Not sure how this did not show up until now.

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2024

Actually one doctest timeout and crash that I thought was caused by singular appears to be linked to mpmath-1.4.0

sage -t --long --random-seed=158424137200168225311908861487827046848 /usr/lib/python3.12/site-packages/sage/functions/orthogonal_polys.py  # Timed out

it seems to get stuck somewhere in gmpy, I have huge backtrace that I am not sure is very useful, but some of the interesting bits of the logs below

sage: chebyshev_T(1234.5, I)                                                # needs sage.rings.real_mpfr sage.symbolic ## line 773 >
-1.21629397684152e472 - 1.21629397684152e472*I
sage: chebyshev_T._evalf_(10^6, 0.1)                                        # needs sage.rings.real_mpfr ## line 779 ##
------------------------------------------------------------------------
/usr/lib/python3.12/site-packages/cysignals/signals.cpython-312-x86_64-linux-gnu.so(+0xa0d4)[0x7f9ee7ade0d4]
/usr/lib/python3.12/site-packages/cysignals/signals.cpython-312-x86_64-linux-gnu.so(+0xa196)[0x7f9ee7ade196]
/usr/lib/python3.12/site-packages/cysignals/signals.cpython-312-x86_64-linux-gnu.so(+0xcba1)[0x7f9ee7ae0ba1]
/usr/lib64/libc.so.6(+0x3c2a0)[0x7f9ee885d2a0]
/usr/lib64/libgmp.so.10(__gmpn_mul_basecase_coreisbr+0x3ac)[0x7f9ee82e506c]
------------------------------------------------------------------------

@skirpichev
Copy link

skirpichev commented Aug 28, 2024

git blame says that value has been around for 13 years in sage and mpmath 1.3.0 has been in sage since July 2023.

Anyway, new answer seems to be correct. Also, Mathematica:

In[4]:= N[Re[CosIntegral[I]], 29]

Out[4]= 0.83786694098020824089467857944

it seems to get stuck somewhere in gmpy

No, it's not gmpy2-related. But in 1.3.0 it quickly quits with NoConvergence exception, so there is a regression. Edit: mpmath/mpmath#852

skirpichev added a commit to skirpichev/mpmath that referenced this pull request Aug 29, 2024
@orlitzky
Copy link
Contributor

Can we do the new version requirement separately? 1.4.0_alpha1 showed up in Gentoo and gets picked up by ./configure.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 18, 2024 via email

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 27, 2024
sagemathgh-38691: build/pkgs/mpmath/version_requirements.txt: Reject 1.4
    
Cherry-picked from sagemath#38565

Gentoo has mpmath-1.4.0_alpha1 in the tree now, and we don't want
`./configure` to use it yet.
    
URL: sagemath#38691
Reported by: Michael Orlitzky
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 28, 2024
sagemathgh-38691: build/pkgs/mpmath/version_requirements.txt: Reject 1.4
    
Cherry-picked from sagemath#38565

Gentoo has mpmath-1.4.0_alpha1 in the tree now, and we don't want
`./configure` to use it yet.
    
URL: sagemath#38691
Reported by: Michael Orlitzky
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants