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

Clean up of MatPow code, forbid non-square matrix powers #19104

Merged
merged 7 commits into from
Apr 16, 2020

Conversation

jlherren
Copy link
Contributor

@jlherren jlherren commented Apr 11, 2020

Completely forbid matrix powers with non-square bases. Further this moves responsibility of evaluating matrix powers to the respective sub-classes via _eval_power().

Other comments

Also includes a fix for Inverse(Inverse(C)).doit()

Release Notes

  • matrices
    • Powers of non-square matrices now always raise an error, even when constructed using MatPow(...).

Inverse(Inverse(A)).doit() was previously returning A ** 1.  This also moves
the responsibility of inverting a MatPow to the MatPow._eval_power method
instead of the Inverse class.
This delegates special MatPow behavior to the respective classes instead of
handling them inside MatPow.doit().
@sympy-bot
Copy link

sympy-bot commented Apr 11, 2020

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:

  • matrices
    • Powers of non-square matrices now always raise an error, even when constructed using MatPow(...). (#19104 by @jlherren)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

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.

Completely forbid matrix powers with non-square bases. Further this moves responsibility of evaluating matrix powers to the respective sub-classes via _eval_power().

#### Other comments
Also includes a fix for Inverse(Inverse(C)).doit()

#### Release Notes
<!-- BEGIN RELEASE NOTES -->
* matrices
  * Powers of non-square matrices now always raise an error, even when constructed using `MatPow(...)`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

(A x B) ** m is no longer transformed to A ** m x B ** m for non-square
matrices A, B, where x is the kronecker product.
@codecov
Copy link

codecov bot commented Apr 11, 2020

Codecov Report

Merging #19104 into master will decrease coverage by 0.016%.
The diff coverage is 92.592%.

@@              Coverage Diff              @@
##            master    #19104       +/-   ##
=============================================
- Coverage   75.786%   75.770%   -0.017%     
=============================================
  Files          650       650               
  Lines       168933    168931        -2     
  Branches     39831     39826        -5     
=============================================
- Hits        128029    128000       -29     
- Misses       35332     35352       +20     
- Partials      5572      5579        +7     

@asmeurer
Copy link
Member

You can add a release notes entry for this.

@oscarbenjamin
Copy link
Contributor

With this we have:

In [14]: MatPow(A, 1)                                                                                                             
Out[14]: 
 1
A 

In [15]: MatPow(A, 1).doit()                                                                                                      
Out[15]: 
 1
A 

In [16]: A**1                                                                                                                     
---------------------------------------------------------------------------
NonSquareMatrixError

I think that it's important that the outcome of MatPow(A, n).doit() is the same as A ** n (which would ideally be implemented simply as MatPow(...).doit()).

Is it possible to raise NonSquareMatrixError on MatPow(A, 0) and MatPow(A, 1) for non-square A or would that break things?

@oscarbenjamin
Copy link
Contributor

Is it possible to raise NonSquareMatrixError on MatPow(A, 0) and MatPow(A, 1) for non-square A or would that break things?

Alternatively could we raise on doit?

@jlherren
Copy link
Contributor Author

@oscarbenjamin Actually that was my original intent, but see https://groups.google.com/forum/#!topic/sympy/o622ArN_F1g

However raising an error in doit() is maybe an option.

sympy/matrices/common.py Outdated Show resolved Hide resolved
@jlherren jlherren changed the title Clean up of MatPow code, forbid most non-square matrix powers Clean up of MatPow code, forbid non-square matrix powers Apr 13, 2020
def test_combine_powers():
assert (C ** 1) ** 1 == C
assert (C ** 2) ** 3 == MatPow(C, 6)
assert (C ** -2) ** -3 == MatPow(C, 6)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be good to have tests showing that the equivalents of these with MatPow do not evaluate to anything e.g.:

from sympy.core.expr import unchanged

C2 = MatPow(C, 2)
assert unchanged(MatPow, C, 0)
assert unchanged(MatPow, C, 1)
assert unchanged(MatPow, C, -1)
assert unchanged(MatPow, C2, 2)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there are no tests at all like that in the MatPow tests. The idea of unchanged is that it asserts that the expression does not evaluate or simplify in any way which is the intention here e.g.:

In [7]: MatPow(Identity(2), 2)                                                                                                    
Out[7]: 
   2
(𝕀) 

In [8]: MatPow(ZeroMatrix(2, 2), 2)                                                                                               
Out[8]: 
   2
(𝟘) 

@oscarbenjamin
Copy link
Contributor

I don't know whether this should raise:

In [9]: MatPow(ZeroMatrix(2, 2), -1)                                                                                              
Out[9]: 
   -1
(𝟘) 

For a non-square matrix it raises before calling doit:

In [4]: MatPow(ZeroMatrix(2, 3), -1)                                                                                              
---------------------------------------------------------------------------
NonSquareMatrixError

@sylee957 sylee957 merged commit 16143cb into sympy:master Apr 16, 2020
@jlherren jlherren deleted the matpower branch April 16, 2020 08:19
@oscarbenjamin
Copy link
Contributor

Thanks @jlherren. This is good.

I still think that this #19104 (comment) needs addressing but I'll open a new issue.

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

Successfully merging this pull request may close these issues.

None yet

6 participants