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

Adapt Mathics interface to SymPy upgrade from 1.8 to 1.10 (resp.1.9) #33584

Closed
soehms opened this issue Mar 28, 2022 · 12 comments
Closed

Adapt Mathics interface to SymPy upgrade from 1.8 to 1.10 (resp.1.9) #33584

soehms opened this issue Mar 28, 2022 · 12 comments

Comments

@soehms
Copy link
Member

soehms commented Mar 28, 2022

Since this upgrade of SymPy (especially by this commit) the Mathics interface is broken:

sage: slist = [[1, 2], 3., 4 + I]
sage: mlist = mathics(slist)
sage: mlist.sage()
Traceback (most recent call last):
...
NotImplementedError: conversion to SageMath is not implemented

Here we implement a fix of that.

Component: interfaces

Keywords: Mathics inteface SymPy

Author: Sebastian Oehms

Branch/Commit: 4302109

Reviewer: Matthias Koeppe

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

@soehms soehms added this to the sage-9.6 milestone Mar 28, 2022
@soehms
Copy link
Member Author

soehms commented Mar 28, 2022

Branch: u/soehms/mathics_sympy_upgr_33584

@soehms

This comment has been minimized.

@soehms
Copy link
Member Author

soehms commented Mar 28, 2022

comment:2

No additional tests needed, since they are already there!

@soehms
Copy link
Member Author

soehms commented Mar 28, 2022

Commit: 4302109

@soehms
Copy link
Member Author

soehms commented Mar 28, 2022

Author: Sebastian Oehms

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 28, 2022

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 28, 2022

comment:3

LGTM

@soehms
Copy link
Member Author

soehms commented Mar 28, 2022

comment:4

Thanks!

@sheerluck
Copy link
Contributor

comment:5

I use this patch

--- a/sage/interfaces/sympy.py
+++ b/sage/interfaces/sympy.py
@@ -259,6 +259,12 @@
         return SR.var(str(self))
 
 
+def _sympysage_mathics_expression(self):
+    from sage.interfaces.mathics import reduce_load
+    return [reduce_load(x)._sage_()
+            for x in self.expr.to_python()]
+
+
 def _sympysage_Subs(self):
     """
     EXAMPLES::
@@ -302,7 +308,7 @@
 
         else:
             # the function defined in sympy is not known in sage
-            raise AttributeError
+            raise AttributeError(fname)
     return func
 
 # the convoluted class structure with metaclasses and stuff sympy uses
@@ -941,6 +947,8 @@
     from sympy.polys.rootoftools import CRootOf
     from sympy.series.order import Order
     from sympy.matrices import ImmutableMatrix, ImmutableSparseMatrix, Matrix, SparseMatrix
+    from mathics.core.convert import SympyExpression
+    SympyExpression._sage_ = _sympysage_mathics_expression
 
     Float._sage_ = _sympysage_float
     Integer._sage_ = _sympysage_integer

but I am not saying that it's correct so I couldn't show it to anybody

@soehms
Copy link
Member Author

soehms commented Mar 29, 2022

comment:6

Replying to @sheerluck:

I use this patch

--- a/sage/interfaces/sympy.py
+++ b/sage/interfaces/sympy.py
@@ -259,6 +259,12 @@
         return SR.var(str(self))
 
 
+def _sympysage_mathics_expression(self):
+    from sage.interfaces.mathics import reduce_load
+    return [reduce_load(x)._sage_()
+            for x in self.expr.to_python()]
+
+
 def _sympysage_Subs(self):
     """
     EXAMPLES::
@@ -302,7 +308,7 @@
 
         else:
             # the function defined in sympy is not known in sage
-            raise AttributeError
+            raise AttributeError(fname)
     return func
 
 # the convoluted class structure with metaclasses and stuff sympy uses
@@ -941,6 +947,8 @@
     from sympy.polys.rootoftools import CRootOf
     from sympy.series.order import Order
     from sympy.matrices import ImmutableMatrix, ImmutableSparseMatrix, Matrix, SparseMatrix
+    from mathics.core.convert import SympyExpression
+    SympyExpression._sage_ = _sympysage_mathics_expression
 
     Float._sage_ = _sympysage_float
     Integer._sage_ = _sympysage_integer

but I am not saying that it's correct so I couldn't show it to anybody

I agree that this fixes the issue, as well. Probably you know better about Mathics than I do, but it seems to be more restrictive, since it converts each instance of SympyExpression to a list. Furthermore, the import statement may fail, since Mathics is an optional package. Thus, it has to be checked that the feature is present.

Anyway, you are welcome to contribute to the code. But I would prefer to do that on a separate ticket, to have the issue fixed, as soon as possible.

@sheerluck
Copy link
Contributor

comment:7

I agree that my patch is a dirty hack :)

I agree that proper work with SympyExpression should be on on a separate ticket.

I agree that this ticket should be merged ASAP.

@vbraun
Copy link
Member

vbraun commented Apr 2, 2022

Changed branch from u/soehms/mathics_sympy_upgr_33584 to 4302109

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

No branches or pull requests

4 participants