-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Faster matrix exponentiation #18595
Faster matrix exponentiation #18595
Conversation
abhinav-anand-addepar
commented
Feb 7, 2020
- matrices
- Faster Matrix exponentiation using Cayley Hamilton Theorem
✅ Hi, I am the SymPy bot (v149). 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.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.
Update The release notes on the wiki have been updated. |
When A = Matrix([[2, 3, 4, 4, 5], [2, 1, 2, 3, 1], [4, 3, 5, 0, 2], [3, 1, 4, 0, 3], [3, 2, 3, 4, 3]]) |
I think the condition check for n can be removed. I will have to do the timing analysis for different rows |
When A = Matrix([[2, 1, 3, 0, 0, 2, 3], [2, 3, 1, 3, 3, 2, 3], [3, 3, 2, 3, 1, 2, 2], [2, 0, 0, 0, 3, 0, 1], [2, 2, 3, 0, 0, 3, 3], [0, 2, 2, 3, 1, 3, 1], [3, 3, 3, 3, 0, 0, 1]]) |
Generally, I don't want to see another polynomial long division reimplemented here Is it slow? |
I am not using polynomial long division |
But I mean what about contributing to the polynomial for more general use I've already found some bug with a matrix with complex entries |
And I ask you what kind of method did you use for computing the polynomial remainer |
There is no polynomial remainder here. I am using this algorithm. |
from sympy import *
A = Matrix([[2+I, 3, 4, 4, 5], [2, 1, 2, 3, 1], [4, 3, 5, 0, 2], [3, 1, 4, 0, 3], [3, 2, 3, 4, 3]])
A._pow_cayley(100) I'm calling from the private function In my second thought, I suppose it can be used for symbolic or complex coefficients, but it should be fixed. |
And I still think that what you're doing here is computing the remainder of |
the problem was that i initialised the polynomial without telling the variable. I used |
Yes i think you are right, let me check which is faster. |
@sylee957 i checked. polynomial division is too slow to be used here. where exp > 1000, i found that there was significant drop in performance when we use polynomial division. |
In my opinion, I think that the idea from here can be generalized to improve the polynomial remainder with a monic polynomial. |
this is a special case. here x**n can be divided by a multivariate polynomial. |
You should use dummy if you are going to use symbols in library code. But generally, I don't think that it's a right direction unless the problem is too difficult. |
sympy/matrices/common.py
Outdated
@@ -2377,7 +2454,9 @@ def __pow__(self, exp): | |||
|
|||
return self.pow(exp) | |||
|
|||
def pow(self, exp, jordan=None): | |||
|
|||
def pow(self, exp, jordan=None, cayley=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having method='jordan'
is better than two boolean arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pow method here is new since the last release of sympy so the signature can be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I’d also want to use a single keyword to control every methods
This looks like solving the linear recursion defined by the characteristic function. The function
Then
|
sympy/discrete/recurrences.py
Outdated
@@ -6,7 +6,7 @@ | |||
from sympy.core import S, sympify | |||
from sympy.core.compatibility import as_int, iterable | |||
|
|||
def linrec(coeffs, init, n): | |||
def linrec(coeffs, init, n, final_expr=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about having a separate linrec_coeffs(coeffs, n)
that could also be used by linrec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would also agree on @jksuom 's idea to have a separate function than adding up a keyword.
It is not a good design because setting the keyword True
would make the variable init
obscure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you look at the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
Codecov Report
@@ Coverage Diff @@
## master #18595 +/- ##
============================================
- Coverage 75.585% 75.58% -0.006%
============================================
Files 644 644
Lines 167452 167573 +121
Branches 39462 39502 +40
============================================
+ Hits 126570 126652 +82
- Misses 35364 35391 +27
- Partials 5518 5530 +12 |
sympy/discrete/recurrences.py
Outdated
|
||
return b[n] if n < k else sum(u*v for u, v in zip(_linrec_coeffs(c, n), b)) | ||
|
||
def _linrec_coeffs(c, n): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could be explicitly public (no _) though not necessarily imported in __init__.py
. The docstring could refer to linrec
.
sympy/matrices/common.py
Outdated
be used if possible and False means it should not be used unless | ||
it is the only way to calculate the power. | ||
|
||
method : jordon, cayley |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also be possible to specify recursion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps recursion should be specified as "multiply" though since I think that makes more sense to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually since dotprodsimp is only used for recursion it should probably be:
method : multiply, mulsimp, jordon, cayley
Then mulsimp corresponds to using recursion with dotprodsimp and the dotprodsimp parameter can be removed (it was added since last release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the changes. Is is good now?
sympy/matrices/common.py
Outdated
def pow(self, exp, jordan=None, dotprodsimp=None): | ||
|
||
def pow(self, exp, method=None): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sympy/matrices/common.py
Outdated
If left as None then Jordan form exponentiation will be used under | ||
certain conditions, True specifies that jordan_pow should always | ||
be used if possible and False means it should not be used unless | ||
it is the only way to calculate the power. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why the tests are failing, could anyone take a look |
got it |
7c0607c
to
058fe50
Compare
sympy/matrices/common.py
Outdated
raise | ||
|
||
if _get_intermediate_simp_bool(True, dotprodsimp): | ||
if method == 'cayley': | ||
return a._eval_pow_by_cayley(exp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These things should outside the enclosing if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Is this okay?
return a._eval_pow_by_recursion_dotprodsimp(exp) | ||
elif method == 'cayley': | ||
if not exp.is_Number or exp % 1 != 0: | ||
raise ValueError("cayley method is only valid for integer powers") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonnegative integer powers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the power is negative, then the matrix is inverted
and exp *= -1
is done in previous step.
So, even method = multiply
works for negative powers.