Revival of crash during FullSimplify
#1156
Comments
|
Unfortunately, another instance, this time stemming from Full tracebackSeems like this problem needs a good, long-term solution. |
This does not crash in the current master branch:
I am not seeing this crash in the current master on github:
I don't have a good high-level understanding of what the problem is. Saying this example crashes or that example crashes, doesn't elucidate what long-term solution is needed. |
Help is always appreciated. I think the thing most needed right now a good high-level description of what the problem is. Is the problem in how things get translated to SymPy? If so, is this a fundamental mismatch? The Mathics-specific There is a newer and even better way to get SymPy call information using an experimental debugger I am working on, but this probably is not yet ready for general use. |
Please don't take this issue and the above statement personally. I only meant that the proposed workaround above may not be sufficient. You're doing a great job and this is a great software that I've started using for my day-to-day work as a replacement for Mathematica, and that says something about the software and its stability already.
I tried to use (mathics) $ mathics -e 'A = Array[a, {3,3}]; Eigenvalues[A.ConjugateTranspose[A]]'
--Return--
> ~/.conda/envs/mathics/lib/python3.11/site-packages/Mathics3-7.0.0-py3.11.egg/mathics/core/convert/sympy.py(166)__new__()->None
-> breakpoint()
(Pdb) list
161
162 def __new__(cls, *args):
163 try:
164 return SympyExpression(self.expr)
165 except Exception:
166 -> breakpoint()
167 # return SympyExpression(expression.Expression(self.expr.head,
168 # *(from_sympy(arg) for arg in args[1:])))
169
170 return SympyExpressionFunc
171
(Pdb) self
SympyExpression(_Mathics_User_Global`a, 2, 1)
(Pdb) isinstance(self, BasicSympy)
True
(Pdb) isinstance(self, Expression)
False
(Pdb) self.expr
*** AttributeError: 'SympyExpression' object has no attribute 'expr'
(Pdb)It seems that the call File "~/.conda/envs/mathics/lib/python3.11/site-packages/sympy/simplify/powsimp.py", line 117, in powsimp
expr = expr.func(*[recurse(w) for w in expr.args])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^is resulting in a |
Honestly, I don't Please try the current master branch. It might not have a problem you are encountering. The 3x3 array example runs slowly, but that is a different problem. |
Hello rocky, does it make sense to find the root cause of this problem? If this bug is already known then I would just switch to current master. Otherwise, I can spend some time to find the root cause... to the best of my abilities. |
|
@aravindh-krishnamoorthy, we are aware that |
|
@rocky, I got the crash on my laptop, even using the master branch. Maybe this is related to a change in Sympy. In any case, it deserves research. Mathics 7.0.1dev0 |
Merged
|
Indeed, I can confirm that is an issue related to Sympy 1.12, and is fixed by using Sympy 1.13 |
Then the simplest fix would be to require SymPy 1.13 or greater. We support back to Python 3.8 and SymPy 1.13 is supported on Python 3.8. Thoughts? |
mmatera
added a commit
that referenced
this issue
Nov 8, 2024
By now, just adding a test for #1156 to see what does the CI
|
I think I could isolate the fix to Strangely However, I feel that there may be a deeper issue here. This is because, more and more, In our case, the code for mathics-core/mathics/core/convert/sympy.py Lines 132 to 159 in 94d26c2 So, calling mathics-core/mathics/core/convert/sympy.py Line 145 in 94d26c2 self.expr is not set: mathics-core/mathics/core/convert/sympy.py Lines 160 to 170 in 94d26c2 Hence, we may have to either remove the handling for It seems that that the handling for |
|
Historically, terminology in Mathics has been weird and this causes a lot of confusion. At least to me, this fuzziness in terminology bleeds into fuzziness in intent as well. Currently, something like "expr" refers to what would be better described as a "compound expression". In other words, an expression that contains subexpressions or is not atomic, such as the name of a variable or a literal constant. The WL term for a node in an expression tree is called an "element", and we have been slowly converting our code to use that term. The intent of SympyExpression's expr attribute, I think, is to have an equivalent representation in Mathics for the SymPy expression. This reduces the need to convert back and forth from Mathics3 to SymPy. If this is correct, then I think this field should be filled in to contain the corresponding Mathics object which here would be a Symbol, or more generally, some kind of Atom. In short, if I have the above correct, the "expr" should be renamed to "element"; it should always be filled in even when the object is not a compound expression. |
Thank you for the detailed explanation. The code in |
|
Hello @mmatera... I've made the following two comments on your commit 94d26c2. Please add them to the code if you feel they are worthwhile. The link to comments is here: 94d26c2#comments The comments are also reproduced below. Comment: This should likely just be Comment: Suggest to also add a numerical test: In[17]:= eigvals /. a[x_, y_] -> x+I*y
Out[17]= 10 - 3 Sqrt[11] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
When using the following on GitHub built Mathics v7.0.0, a crash is observed.
More
How to Reproduce
Output Given
Crash, please see above.
Expected behavior
Simplified expressions for eigenvalues; likely the result obtained in the Workarounds section below.
Your Environment
Mathics 7.0.0
on CPython 3.11.10 | packaged by conda-forge | (main, Oct 16 2024, 01:27:36) [GCC 13.3.0]
using SymPy 1.12.1, mpmath 1.3.0, numpy 2.1.3, cython Not installed
OS: Windows 11 WSL
Workarounds
By changing the line (of v7.0.0 code) from:
mathics-core/mathics/builtin/numbers/algebra.py
Line 1656 in 6a0b3f0
to
analogous to #238, the following output is obtained:
{-Sqrt[(a[1, 1] Conjugate[a[1, 1]] + a[1, 2] Conjugate[a[1, 2]] + a[2, 1] Conjugate[a[2, 1]] + a[2, 2] Conjugate[a[2, 2]]) ^ 2 - 4 a[1, 1] a[2, 2] Conjugate[a[1, 1]] Conjugate[a[2, 2]] - 4 a[1, 2] a[2, 1] Conjugate[a[1, 2]] Conjugate[a[2, 1]] + 4 a[1, 1] a[2, 2] Conjugate[a[1, 2]] Conjugate[a[2, 1]] + 4 a[1, 2] a[2, 1] Conjugate[a[1, 1]] Conjugate[a[2, 2]]] / 2 + a[1, 1] Conjugate[a[1, 1]] / 2 + a[1, 2] Conjugate[a[1, 2]] / 2 + a[2, 1] Conjugate[a[2, 1]] / 2 + a[2, 2] Conjugate[a[2, 2]] / 2, Sqrt[(a[1, 1] Conjugate[a[1, 1]] + a[1, 2] Conjugate[a[1, 2]] + a[2, 1] Conjugate[a[2, 1]] + a[2, 2] Conjugate[a[2, 2]]) ^ 2 - 4 a[1, 1] a[2, 2] Conjugate[a[1, 1]] Conjugate[a[2, 2]] - 4 a[1, 2] a[2, 1] Conjugate[a[1, 2]] Conjugate[a[2, 1]] + 4 a[1, 1] a[2, 2] Conjugate[a[1, 2]] Conjugate[a[2, 1]] + 4 a[1, 2] a[2, 1] Conjugate[a[1, 1]] Conjugate[a[2, 2]]] / 2 + a[1, 1] Conjugate[a[1, 1]] / 2 + a[1, 2] Conjugate[a[1, 2]] / 2 + a[2, 1] Conjugate[a[2, 1]] / 2 + a[2, 2] Conjugate[a[2, 2]] / 2}which is likely correct. Note that the above code is restructured a bit (and modified) in the latest commit as of now:
mathics-core/mathics/eval/numbers/algebra/simplify.py
Line 85 in c5a9227
Priority
Programmable workaround exists.
Additional context
In case the workaround is fine, I can submit a PR. Otherwise, I can also help with a good fix under instructions as I'm new to Mathics.
The text was updated successfully, but these errors were encountered: