matrices: Allow inverse call on symbolic matrix #19217
Conversation
|
✅ Hi, I am the SymPy bot (v158). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7. Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed. |
|
Thanks but I think this misses the actual problem. I don't think that we want a new class to solve this. We just need to fix the bug that stops inverse working for the previous class. |
|
I don't think that it should return unevaluated inverse for explicit matrices, computing the actual inverse should be its own design goal. |
|
The root of the issue is |
|
As noted in #19162 the |
|
@oscarbenjamin What's the reasoning behind adding checks in the constructor? I'm not seeing that .subs creates new instances of the instance it gets called on so I don't think we can raise an exception from the constructor? My Python experience (or lack thereof) may be holding me back here. I can see adding a check for |
|
When you call In [7]: class Thing(Basic):
...: def __new__(self, arg):
...: if isinstance(arg, Symbol):
...: return super().__new__(Thing, arg)
...: else:
...: raise ValueError("Bad arguments")
...:
In [8]: a = Thing(x)
In [9]: a
Out[9]: Thing(x)
In [10]: a.subs(x, y)
Out[10]: Thing(y)
In [11]: a.subs(x, 3)
---------------------------------------------------------------------------
ValueErrorI think that for |
|
If you need an atomic class only for the |
|
@sylee957 are you suggesting we create a class similar to MatrixSymbol with checks in the constructor for the This will raise an exception if I do this: The behavior is the same if I modify MatrixSymbol's constructor. Of course this means we should be catching the exception somewhere. But is that "somewhere" better than the place I've caught the TypeError in Catching the TypeError in expr.py in my latest commit does not change the behavior of is_constant or .equals in expr.py. These methods are meant to try a bunch of different expressions and catch exceptions, trying to determine whether the argument passed to it is a constant or whether two things equal each other. The Let me know what you guys think |
|
The problem is that the exception is being raised in the wrong place and the diff here tries to compensate for that by catching it in the wrong place. The right place to raise this is in the call to subs ( diff --git a/sympy/core/expr.py b/sympy/core/expr.py
index 388595f428..9c7e0159d8 100644
--- a/sympy/core/expr.py
+++ b/sympy/core/expr.py
@@ -675,7 +675,7 @@ def check_denominator_zeros(expression):
if a is S.NaN:
# evaluation may succeed when substitution fails
a = expr._random(None, 0, 0, 0, 0)
- except ZeroDivisionError:
+ except (ZeroDivisionError, TypeError):
a = None
if a is not None and a is not S.NaN:
try:
@@ -684,7 +684,7 @@ def check_denominator_zeros(expression):
if b is S.NaN:
# evaluation may succeed when substitution fails
b = expr._random(None, 1, 0, 1, 0)
- except ZeroDivisionError:
+ except (ZeroDivisionError, TypeError):
b = None
if b is not None and b is not S.NaN and b.equals(a) is False:
return False
diff --git a/sympy/matrices/expressions/matexpr.py b/sympy/matrices/expressions/matexpr.py
index 56f080da5c..229da0e82b 100644
--- a/sympy/matrices/expressions/matexpr.py
+++ b/sympy/matrices/expressions/matexpr.py
@@ -712,6 +712,9 @@ def __new__(cls, name, n, m):
if isinstance(name, str):
name = Symbol(name)
name = _sympify(name)
+ if not isinstance(name, (Symbol, MatrixExpr)):
+ msg = "name should be Symbol/MatrixExpr not %s"
+ raise TypeError(msg % (type(name),))
obj = Expr.__new__(cls, name, n, m)
return objThat gives: In [1]: import sympy as sp
In [2]: X = sp.Matrix(sp.MatrixSymbol('X', 2, 2))
In [3]: X.inv()
Out[3]:
⎡ X₁₁ -X₀₁ ⎤
⎢───────────────── ─────────────────⎥
⎢X₀₀⋅X₁₁ - X₀₁⋅X₁₀ X₀₀⋅X₁₁ - X₀₁⋅X₁₀⎥
⎢ ⎥
⎢ -X₁₀ X₀₀ ⎥
⎢───────────────── ─────────────────⎥
⎣X₀₀⋅X₁₁ - X₀₁⋅X₁₀ X₀₀⋅X₁₁ - X₀₁⋅X₁₀⎦However I dislike catching generic TypeError there. We should make a new |
|
I may have pushed the latest commit too soon, I see I'm failing some of the tests. Will go check. @sylee957 please feel free to weigh in if you think MatrixElement shouldn't be modified. I'm not sure what you mean by creating an atomic class for MatrixSymbol. |
vanillafrosty
force-pushed
the
19162_inverse_dense_symbolic
branch
from
84ac5c6
to
12d92c0
Compare
|
There is still an issue with my latest commit - KroneckerDelta computation seems to be broken. In line 553 in I don't know enough about KroneckerDelta to set |
|
Does this work? diff --git a/sympy/matrices/expressions/matexpr.py b/sympy/matrices/expressions/matexpr.py
index 56f080da5c..959474cb77 100644
--- a/sympy/matrices/expressions/matexpr.py
+++ b/sympy/matrices/expressions/matexpr.py
@@ -548,9 +548,11 @@ def recurse_expr(expr, index_ranges={}):
elif isinstance(expr, KroneckerDelta):
i1, i2 = expr.args
if dimensions is not None:
- identity = Identity(dimensions[0])
+ size = dimensions[0]
else:
- identity = S.One
+ range1 = index_ranges[i1]
+ size = range1[1] - range1[0] + 1
+ identity = Identity(size)
return [(MatrixElement(identity, i1, i2), (i1, i2))]
elif isinstance(expr, MatrixElement):
matrix_symbol, i1, i2 = expr.argsI'm not sure I understand that function fully though and it does seem like there could be some other problems there... |
|
Yes that works! It looks correct to me, but then again I'm not sure how to write comprehensive tests for that KroneckerDelta computation. |
|
Although substituting MatrixSymbol with scalar constants should be disallowed in any other patches, I don't think that it should rely on error catching. The actual problem is diff --git a/sympy/core/expr.py b/sympy/core/expr.py
index 388595f428..694d2e7372 100644
--- a/sympy/core/expr.py
+++ b/sympy/core/expr.py
@@ -665,13 +665,24 @@ def check_denominator_zeros(expression):
if expr.is_zero:
return True
- # try numerical evaluation to see if we get two different values
+ def zero(wrt):
+ from sympy.matrices.immutable import ImmutableDenseMatrix as Matrix
+ if wrt.is_Matrix:
+ return Matrix.zeros(wrt.rows, wrt.cols)
+ return S.Zero
+
+ def one(wrt):
+ from sympy.matrices.immutable import ImmutableDenseMatrix as Matrix
+ if wrt.is_Matrix:
+ return Matrix.ones(wrt.rows, wrt.cols)
+ return S.One
+
failing_number = None
if wrt == free:
# try 0 (for a) and 1 (for b)
try:
- a = expr.subs(list(zip(free, [0]*len(free))),
- simultaneous=True)
+ subs_dict = {x: zero(x) for x in free}
+ a = expr.subs(subs_dict, simultaneous=True)
if a is S.NaN:
# evaluation may succeed when substitution fails
a = expr._random(None, 0, 0, 0, 0)
@@ -679,8 +690,8 @@ def check_denominator_zeros(expression):
a = None
if a is not None and a is not S.NaN:
try:
- b = expr.subs(list(zip(free, [1]*len(free))),
- simultaneous=True)
+ subs_dict = {x: one(x) for x in free}
+ b = expr.subs(subs_dict, simultaneous=True)
if b is S.NaN:
# evaluation may succeed when substitution fails
b = expr._random(None, 1, 0, 1, 0) |
|
@sylee957 that approach works for me. I've kept the new BadArgumentsError for usage going forward, and took out the place we were catching it in I've added a conditional check for MatrixSymbol, and |
I don't know why that code is there in basic.py but I think it's been problematic in other situations so maybe there's a way to get rid of it. |
|
The tests have failed because the newline was removed at the end of basic.py |
Fixes #19162
A symbolic matrix A whose elements can be retrieved by indexing into A (A[i], where i is an integer),
is now invertible after A is made explicit with A.as_explicit(). This will return a new subclass of
ImmutableDenseMatrix, where only the inverse method is overwritten.
For example:
A = MatrixSymbol('A', 2, 2)
exA = A.as_explicit()
exA.inv()
Now works.
Release Notes