Bug Fixed : Matrix power of identity matrix fails #9823 #9830

kevalds51 · 2015-08-14T23:48:12Z

Added the test to exercise the code change. (as requested by the @moorepants )
Modified the code such that no error is generated for both MatPow method and ** operator.
removed extra blank line which resulted in test_code_quality fail.

bjodah · 2015-08-17T18:02:10Z

I think this looks fine and is a nice addition, one peculiar inconsistency I noticed though:

>>> I3=sympy.Identity(3)
>>> I3**3
I
>>> I3**-1
I^-1
>>> (I3**-1).doit()
I

i.e. for negative powers the doit() call is neccessary for simplification.
Maybe there is a good reason for this?

kevalds51 · 2015-08-17T19:23:21Z

Thanks for pointing that out @bjodah
This can be easily solved by changing the position of the elif condition in line 119-120 and putting it above the 'elif other is isInverse' statement

I will make the change and update the pull request.
Cheers

oliverlee · 2015-08-17T19:36:40Z

@bjodah @kevalds51 This problem also occurs with ZeroMatrix as MatPow only explicitly evaluates instances of MatrixBase: #9823 (comment)

moorepants · 2015-08-19T18:16:42Z

@kevalds51, @oliverlee and I just spoke. We can accept this PR instead of Oliver's but you need to fix the ZeroMatrix issue here too and add tests for it. Maybe see Oliver's PR for how he did it.

kevalds51 · 2015-08-19T19:36:21Z

@moorepants
Thanks for the advice, i'll start working on it right off.

kevalds51 · 2015-08-20T19:35:32Z

@moorepants I am getting this test_code_quality fail, can you suggest me on how to cope up with it.

leosartaj · 2015-08-20T19:46:33Z

sympy/matrices/expressions/tests/test_matpow.py

+    assert MatPow(l, 2).doit() == l
+    assert MatPow(l, -1).doit() == l
+    assert MatPow(l, 0).doit() == l
+


You have trailing spaces on this line. FYI you should run ./bin/test sympy/utilities/tests/test_code_quality.py the output will tell you about the code quality issues.

Here's what I get

$ ./bin/test sympy/utilities/tests/test_code_quality.py ================================================================ test process starts ================================================================= executable: /home/leosartaj/projects/sympy/my/bin/python (2.7.10-final-0) [CPython] architecture: 64-bit cache: yes ground types: python random seed: 56687482 hash randomization: on (PYTHONHASHSEED=2022273946) sympy/utilities/tests/test_code_quality.py[6] F..... [FAIL] ______________________________________________________________________________________________________________________________________________________ _______________________________________________ sympy/utilities/tests/test_code_quality.py:test_files ________________________________________________ File "/home/leosartaj/projects/sympy/sympy/utilities/tests/test_code_quality.py", line 228, in test_files check_directory_tree(SYMPY_PATH, test, exclude) File "/home/leosartaj/projects/sympy/sympy/utilities/tests/test_code_quality.py", line 107, in check_directory_tree check_files(glob(join(root, pattern)), file_check, exclusions) File "/home/leosartaj/projects/sympy/sympy/utilities/tests/test_code_quality.py", line 123, in check_files file_check(fname) File "/home/leosartaj/projects/sympy/sympy/utilities/tests/test_code_quality.py", line 146, in test test_this_file(fname, test_file) File "/home/leosartaj/projects/sympy/sympy/utilities/tests/test_code_quality.py", line 169, in test_this_file assert False, message_space % (fname, idx + 1) AssertionError: File contains trailing whitespace: /home/leosartaj/projects/sympy/sympy/matrices/expressions/tests/test_matpow.py, line 108. ================================================ tests finished: 5 passed, 1 failed, in 2.88 seconds ================================================= DO *NOT* COMMIT!

kevalds51 · 2015-08-21T05:58:03Z

@moorepants Is this ok to be merged? can you please review this again and tell me.

@leosartaj I appreciate it, that trailing space troubled me a lot earlier.

moorepants · 2015-08-21T07:26:26Z

sympy/matrices/expressions/tests/test_matpow.py

@@ -70,7 +70,7 @@ def test_doit_square_MatrixSymbol_symsize():

 def test_doit_with_MatrixBase():
    X = ImmutableMatrix([[1, 2], [3, 4]])
-    assert MatPow(X, 0).doit() == ImmutableMatrix(Identity(2))
+    assert MatPow(X, 0).doit().as_explicit() == ImmutableMatrix(Identity(2))


Why is the test changed? You shouldn't change a test unless it was testing against an incorrect solution.

You're new code should not affect existing tests (unless the tests were wrong), it should only need new tests added.

@moorepants
I did it because ImmutableMatrix(Identity(2)) returns the matrix in explicit form. (So i guess the test is wrong.)
So inspite of ImmutableMatrix(Identity(2)), it can be just : Identity(2).
or we can just check the explicit form of LHS term

moorepants · 2015-08-21T07:27:33Z

@oliverlee Can you review this since you thought about it more?

oliverlee · 2015-08-21T10:31:01Z

We want any powers of Identity and positive powers of (square) ZeroMatrix to be equal to themselves, not just the explicit representation.

Test functions names should use Identity and ZeroMatrix as those are the classes being tested and to conform with other written tests.

Also, I wasn't thinking when writing my tests, but the inverse of ZeroMatrix is not ZeroMatrix.

jksuom · 2015-08-21T10:35:22Z

Should the 0'th power of ZeroMatrix also be defined?

oliverlee · 2015-08-21T10:38:34Z

It is here:

sympy/sympy/matrices/expressions/matexpr.py

Line 466 in 5578492

if other == 0:

jksuom · 2015-08-21T10:40:21Z

OK, that's what I had in mind.

kevalds51 · 2015-08-21T12:04:41Z

@oliverlee
Ill take that very important observation into consideration.
Thanks

kevalds51 · 2015-08-22T12:46:26Z

I don't understand why the most recent commit is failing, can anyone please help.

leosartaj · 2015-08-22T12:48:53Z

This is probably due to #9846. This is fixed now. You should update your branch with the current master.

kevalds51 · 2015-08-22T19:18:12Z

@oliverlee @moorepants
Does this look good now?

oliverlee · 2015-08-24T14:46:02Z

sympy/matrices/expressions/tests/test_matpow.py

@@ -70,7 +70,7 @@ def test_doit_square_MatrixSymbol_symsize():

 def test_doit_with_MatrixBase():
    X = ImmutableMatrix([[1, 2], [3, 4]])
-    assert MatPow(X, 0).doit() == ImmutableMatrix(Identity(2))
+    assert MatPow(X, 0).doit() == Identity(2)


This test shouldn't change. If you take an instance of ImmutableMatrix to the zero power, you expect to get back an ImmutableMatrix.

oliverlee · 2015-08-24T14:51:48Z

The use of as_explicit() in your ZeroMatrix and Identity tests seem unnecessary; positive powers should return the same class.
I don't think the result of taking an ImmutableMatrix to the zero power should be a different class but maybe someone else who is more familiar with the matrix stuff can weigh in.
I'm not sure how errors should be handled with regards to the ZeroMatrix taken to negative powers. Maybe a separate issue should be opened for that?

Other than that, looks fine to me.

kevalds51 · 2015-08-24T16:24:03Z

@oliverlee
So should I just remove the unnecessary test cases? Is that all?

oliverlee · 2015-08-26T09:31:24Z

@kevalds51 Sure, and also revert changes to that one test case. I don't know how errors should be raised though.

kevalds51 · 2015-08-26T12:29:10Z

@oliverlee
The issue with the test case is that it returns matrix in explicitly! This leads to an assertion error.
So either I have to change the RHS to Identity(2) or test for LHS explicitly as shown below

X = ImmutableMatrix([[1, 2], [3, 4]])
assert MatPow(X, 0).doit() == Identity(2)

or

X = ImmutableMatrix([[1, 2], [3, 4]])
assert MatPow(X, 0).doit().as_explicit() == ImmutableMatrix(Identity(2))

Can you or @moorepants help me with this

oliverlee · 2015-08-26T13:33:49Z

@kevalds51
In MatPow.doit(), a base argument that is an instance of MatrixBase will always result in a call to base.__pow__(exp), unless the exponent is 1. In either case, an instance of MatrixBase is returned.

Your patch always returns an instance of Identity if the exponent is zero without taking the class of the base into account, which is change from the previous behavior.

kevalds51 · 2015-08-26T20:20:51Z

@oliverlee How about this?
I guess this commit is quite suitable, and meets all the requirements specified so far.
Good enough to merge?

kevalds51 · 2015-08-29T12:14:15Z

@bjodah can you please review this?

jksuom · 2015-08-29T12:41:45Z

sympy/matrices/expressions/matexpr.py

@@ -110,7 +110,11 @@ def __rmul__(self, other):
    def __pow__(self, other):
        if not self.is_square:
            raise ShapeError("Power of non-square matrix %s" % self)
-        if other is S.NegativeOne:
+        if self.is_ZeroMatrix:
+            return self


This seems to return the zero matrix for all powers while line 467 returns the identity for the 0th power of the zero matrix.

@jksuom That is because, zero matrix raised to any positive power is zero matrix itself, whereas zero matrix raised to power 0 is an Identity matrix.

Will the exponent other be positive here?

Yes.
If an instance of a zero matrix is raised to -ve power it will raise the value error saying matrix is invertible, as in : (ZeroMatrix(3,3))**-2

It seems that ZeroMatrix.__pow__ already takes care of all powers of a ZeroMatrix: zeroth, negative (could be if other < 0:) and positive. Under which conditions would a ZeroMatrix appear as an argument of MatrixExpr.__pow__ making this line necessary?

After running a few checks, I agree to you that this line is redundant, I will make the changes as soon as possible.

Earlier I wasn't able to get you, sorry for the misunderstanding.

jksuom · 2015-08-29T19:23:57Z

It's a PyPy issue. I have restarted the job.

kevalds51 · 2015-08-29T19:41:23Z

@jksuom All good now?

gxyd · 2015-08-30T02:20:51Z

sympy/matrices/expressions/matexpr.py

@@ -465,6 +467,8 @@ def __pow__(self, other):
            raise ShapeError("Power of non-square matrix %s" % self)
        if other == 0:
            return Identity(self.rows)
+        if other<1:


other<1 -> other < 1 , space needed

gxyd · 2015-08-30T02:24:20Z

You can insert space at all other places as well, it makes the code more of readable.

gxyd · 2015-08-30T05:36:43Z

sympy/matrices/expressions/tests/test_matpow.py

+    assert MatPow(l, 2).doit() == l
+    assert MatPow(l, -1).doit() == l
+    assert MatPow(l, 0).doit() == l
+


One more line here.

gxyd · 2015-08-31T19:07:51Z

@kevalds51 you should take a look at this comment.

kevalds51 · 2015-09-02T07:02:12Z

@oliverlee You want to review this one more time?

kevalds51 · 2015-09-03T13:29:07Z

@moorepants does this look good?

oliverlee · 2015-09-04T17:02:21Z

sympy/matrices/expressions/matpow.py

@@ -56,14 +56,25 @@ def doit(self, **kwargs):
            args = self.args
        base = args[0]
        exp = args[1]
-        if isinstance(base, MatrixBase) and exp.is_number:
+        if exp.is_zero and base.is_square:
+            try:


I'm not sure why you're using a try/catch statement to evaluate an object of instance MatrixBase to the zero power. It's already defined here:

sympy/sympy/matrices/matrices.py

Line 521 in 19f5894

def __pow__(self, num):

But it looks like all tests pass and this addresses the issue.

This was done to deal with special cases, such as instances of Immutable matrix (being raised to power 0), where I have to return it in the form of base.class(Identity(base.shape[0])), and not just as Identity((base.shape[0])).

If not done so, there will be issues while dealing with Immutable matrix instance or other similar ones. (It did lead to assertion errors in test cases in previous commits)

Can't you check if it's an instance of MatrixBase and calculate the result normally? There weren't any problems with that before so I don't see why that wouldn't work here.

To solve the issue in the first place (for powers of both Identity and Zero Matrix), changes were made that required further adding of conditions.
Also there were other examples that gave wrong results for power zero (some were not tested in the test cases), to deal with that I had to add try/catch to return the proper instance.

kevalds51 · 2015-09-14T17:05:20Z

I'm facing some problems in managing branches, so I'll be closing this pull request.

@bjodah Should I create another request (this exact same one) after I fix the issues on my machine?

bjodah · 2015-09-14T17:37:38Z

@kevalds51, there really shouldn't be a need for you to open another pull request. If you do, the people who have reviewed this PR would have to go over everything again from scratch (or cross reference everything from the review here). More specifically, what troubles are you facing with git, maybe we can help?

kevalds51 · 2015-09-14T17:44:52Z

That would be great. I just got used to Git a few weeks back.
Talking of this pull request, I pushed it from the master branch (of the forked repo on my machine). I don't know what to do if I want to work on some other issue...

bjodah · 2015-09-14T19:10:43Z

I see. You should be able to do something along these lines:

$ git remote add upstream git://github.com/sympy/sympy.git
$ git fetch upstream
$ git checkout -b a_new_branch
$ git rebase upstream/master
$ # edit some files, commit the changes and push them to your repo:
$ git push origin --set-upstream a_new_branch
$ # if you want to go back to your master branch (which currently corresponds to this PR):
$ git checkout master

EDIT: added fetch on row 2

kevalds51 · 2015-09-14T20:49:38Z

Worked nicely, thanks

moorepants · 2015-10-09T14:43:25Z

@kevalds51 Any reason you closed this?

kevalds51 · 2015-10-09T14:48:50Z

I had pushed from master of my local repo, this also led to merge conflicts in new PRs.
I have saved this work, do you want me to open another PR for this?

moorepants · 2015-10-09T15:00:41Z

If we haven't gotten your work on this merged, we want to. You can open a new PR if you are having git issues. But in the future, ask us about the git issues and merge conflicts and we can help you solve them.

kevalds51 · 2015-10-09T15:41:58Z

Sure, it is settled once and for all. As of now, I'll open a new PR.

kevalds51 · 2015-10-09T15:50:33Z

Please refer to PR #9978

bjodah mentioned this pull request Aug 17, 2015

Evaluate powers of Identity, ZeroMatrix matrices #9827

Closed

leosartaj reviewed Aug 20, 2015
View reviewed changes

moorepants reviewed Aug 21, 2015
View reviewed changes

oliverlee reviewed Aug 24, 2015
View reviewed changes

jksuom reviewed Aug 29, 2015
View reviewed changes

gxyd reviewed Aug 30, 2015
View reviewed changes

oliverlee reviewed Sep 4, 2015
View reviewed changes

kevalds51 force-pushed the master branch from 21b2c21 to d9d8c21 Compare October 9, 2015 12:44

kevalds51 closed this Oct 9, 2015

kevalds51 force-pushed the master branch from d9d8c21 to 9039504 Compare October 9, 2015 13:58

Bug Fixed : Matrix power of identity matrix fails #9823 #9830

Bug Fixed : Matrix power of identity matrix fails #9823 #9830

Conversation

kevalds51 commented Aug 14, 2015

bjodah commented Aug 17, 2015

kevalds51 commented Aug 17, 2015

oliverlee commented Aug 17, 2015

moorepants commented Aug 19, 2015

kevalds51 commented Aug 19, 2015

kevalds51 commented Aug 20, 2015

Choose a reason for hiding this comment

kevalds51 commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorepants commented Aug 21, 2015

oliverlee commented Aug 21, 2015

jksuom commented Aug 21, 2015

oliverlee commented Aug 21, 2015

jksuom commented Aug 21, 2015

kevalds51 commented Aug 21, 2015

kevalds51 commented Aug 22, 2015

leosartaj commented Aug 22, 2015

kevalds51 commented Aug 22, 2015

Choose a reason for hiding this comment

oliverlee commented Aug 24, 2015

kevalds51 commented Aug 24, 2015

oliverlee commented Aug 26, 2015

kevalds51 commented Aug 26, 2015

oliverlee commented Aug 26, 2015

kevalds51 commented Aug 26, 2015

kevalds51 commented Aug 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jksuom commented Aug 29, 2015

kevalds51 commented Aug 29, 2015

Choose a reason for hiding this comment

gxyd commented Aug 30, 2015

Choose a reason for hiding this comment

gxyd commented Aug 31, 2015

kevalds51 commented Sep 2, 2015

kevalds51 commented Sep 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevalds51 commented Sep 14, 2015

bjodah commented Sep 14, 2015

kevalds51 commented Sep 14, 2015

bjodah commented Sep 14, 2015

kevalds51 commented Sep 14, 2015

moorepants commented Oct 9, 2015

kevalds51 commented Oct 9, 2015

moorepants commented Oct 9, 2015

kevalds51 commented Oct 9, 2015

kevalds51 commented Oct 9, 2015