Enabled commutative property for MatrixElement #11084
Conversation
|
@asmeurer , can you review this? |
|
Looks like some tests failed. Those should be investigated. |
| @@ -338,7 +338,7 @@ class MatrixElement(Expr): | |||
| i = property(lambda self: self.args[1]) | |||
| j = property(lambda self: self.args[2]) | |||
| _diff_wrt = True | |||
|
|
|||
| is_commutative = True | |||
There was a problem hiding this comment.
Keep a blank line before function definitions.
|
You should add a test for the original issue. |
Printing tests which give the correct output but different in string values are modified: example: M[6] = 2*q[4]*1/q[1]; is equivalent to M[6] = 2*q[4]/q[1];
|
I wonder if there could be a way of making matrix elements commutative on demand. Matrices with elements in non-commutative rings are generally quite acceptable. Commutativity is needed only in some special cases such as for determinants. |
As the MatrixElement is_commutative property is set to true, the expression in line 2283 wasn’t being interpreted correctly and the cancel function was returning unsimplified output previously
|
@asmeurer , should I add the tests for the determinant of the matrix (i.e the issue for determinant which u originally posted) or just the commutability of the MatrixElement? |
Added test for commutative property of MatrixElement
|
@asmeurer , can you review this? |
| @@ -2881,7 +2882,7 @@ def test_cancel(): | |||
| M = MatrixSymbol('M', 5, 5) | |||
| assert cancel(M[0,0] + 7) == M[0,0] + 7 | |||
| expr = sin(M[1, 4] + M[2, 1] * 5 * M[4, 0]) - 5 * M[1, 2] / z | |||
| assert cancel(expr) == expr | |||
| assert simplify(cancel(expr)) == expr | |||
There was a problem hiding this comment.
This test seems wrong. The point of the function is to test cancel. What does this do without simplify here?
There was a problem hiding this comment.
I guess it should return (z*sin(M[1, 4] + M[2, 1] * 5 * M[4, 0]) - 5 * M[1, 2]) / z. If that's what it's giving, we should test that.
There was a problem hiding this comment.
yeah it returns cancel(expr) = (z*sin(M[1, 4] + M[2, 1] * 5 * M[4, 0]) - 5 * M[1, 2]) / z and previously it was returning the expression sin(M[1, 4] + 5*M[2, 1]*M[4, 0]) - 5*M[1, 2]/z, please note that the whole numerator is encloses in a bracket after is_commutative is set to True hence I used simplify
There was a problem hiding this comment.
(z*sin(M[1, 4] + M[2, 1] * 5 * M[4, 0]) - 5 * M[1, 2]) / z is the right answer for cancel to return. The test should be
assert cancel(expr) == (z*sin(M[1, 4] + M[2, 1] * 5 * M[4, 0]) - 5 * M[1, 2]) / zCalling simplify in the function that tests the output of cancel defeats the purpose.
|
I would add a test for the commutativity (which you already have), and a test for the determinant, to make sure that that works. |
|
@asmeurer , I added the test for determinant of the matrix. |
|
Just the comment on the |
|
@asmeurer , I fixed the test |
|
@asmeurer , can you review this again? |
|
Looks good, thanks. |
|
Oh, one thing. In the future, if you fix an issue, put "Fixes #11077" in one of your commit messages. That way, when the PR is merged, the issue will be automatically closed. |
This is fix for #11077
Previously the
is_commutativeproperty wasNoneand hence Sympy wasnt able to simplify the determinant expression.is_commutativeproperty toTrue, the determinant expression is being simplified.is_commutativeproperty