| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fixed 'simplify' keyword not working in eigenvals() #15125
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
Conversation
|
✅ Hi, I am the SymPy bot (v129). 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.2.1. 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. |
|
|
|
sympy/sympy/matrices/matrices.py Line 293 in c6cf691
I think that the flag stands for function name, not a boolean. |
Yes, it does. But I think that it might sometimes be desirable to change the method. Maybe the argument |
|
@jksuom I have made my custom key
You can see how simplify() applied at each level of function call stack significantly affects performance, and I would want to hear your opinions about this, before moving on to working on compatibility with other functions |
|
I am not too much worried about the time needed to find eigenvalues/vectors (assuming that it is only a few seconds). I think that most users primarily want to get the results in a convenient form even if that would take a little longer. There should probably be at most one simplification. Maybe |
|
As a user who stumbled upon this problem, I have to admint that neither speed nor the aesthetic appeal are crucial to me. My primary problem was that the function .eigenvects(simplify=True) failed with a NotImplementedError, claiming that it could not compute eigenvectors for the eigenvalues (displaying them as not simplified). It is perfectly fine for me as a user if the computation for both eigenvalues and eigenvectors succeeds with ugly (not simplified) results. Performing simplifications a posteriori is good enough. But if the lack of simplifications in the eigenvalues causes no result in eigenvectors, this is utmost frustrating. So when trying to find a good default behavior for eigenvals(simplify=True), this should be done in coordination with the behavior of eigenvects(simplify=True), so as to at least circumvent the NotImplementedError and give some result for the eigenvectors. Especially for the simple matrix mentioned in #15118. |
|
@adoa However, after tracing down your issue, I have also found that the And I also could not solve your problem in my Mathematica, and am getting some weird results from it. |
|
In Mathematica I used a global positivity assumption for q in the form of |
|
Mathematica's |
My Progress so farI had mapped However, the result from applying
And another surprising thing is that the result from the first case is the only one which can succeed in computing eigenvects, from the case provided from @adoa
Conclusion at the momentSince there is some unclear reason that simplify executed in every step makes result look more cleaner than using lazy evaluation of simplify, I may suggest that Any ideas? |
sympy/matrices/matrices.py
Outdated
| If simplify is set to False, it will skip simplification in this | ||
| particular routine to save computation resources. | ||
| If you pass a function to simplify, it will attempt to apply | ||
| the partucular function as simplification method. |
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.
-> particular
| @@ -1069,6 +1069,11 @@ def NS(e, n): | |||
| assert count_ops(m.eigenvals(simplify = False)) > count_ops(m.eigenvals(simplify = True)) | |||
| assert count_ops(m.eigenvals(simplify = lambda x: x)) > count_ops(m.eigenvals(simplify = True)) | |||
|
|
|||
| assert isinstance(m.eigenvals(simplify = True, multiple=False), dict) | |||
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 tries to conform to PEP 8 in new code:
Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.
|
@sylee957 If you close and reopen the PR, all build jobs will be restarted. If only one or a few fail because of a connection failure or other timeout failure, it is more efficient to restart only those. You can ask for someone to restart either in a comment here or on gitter. |
sympy/matrices/matrices.py
Outdated
| @@ -1141,15 +1164,25 @@ def eigenvals(self, error_when_incomplete=True, **flags): | |||
| eigs[diagonal_entry] += 1 | |||
| else: | |||
| flags.pop('simplify', None) # pop unsupported flag | |||
| eigs = roots(mat.charpoly(x=Dummy('x')), **flags) | |||
| eigs = roots(mat.charpoly(x=Dummy('x'), simplify=simpfunc_1), **flags) | |||
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 if isinstance(simplify, FunctionType): could be here. Then charpoly could be called with simplify=simplify. Otherwise the line above would be used.
sympy/matrices/matrices.py
Outdated
| # simplify() function will be applied once at the end of the routine. | ||
| if not simplify: | ||
| return eigs | ||
| else: |
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.
This could be
if not isinstance(simplify, FunctionType):
simplify = _simplify
Then we could use simplify instead of simpfunc_2. (else: and indentation is not necessary.)
sympy/matrices/matrices.py
Outdated
| @@ -1164,7 +1153,7 @@ def eigenvals(self, error_when_incomplete=True, **flags): | |||
| eigs[diagonal_entry] += 1 | |||
| else: | |||
| flags.pop('simplify', None) # pop unsupported flag | |||
| eigs = roots(mat.charpoly(x=Dummy('x'), simplify=simpfunc_1), **flags) | |||
| eigs = roots(mat.charpoly(x=Dummy('x'), simplify=simplify if isinstance(simplify, FunctionType) else _simplify), **flags) | |||
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 would prefer having
if isinstance(simplify, FunctionType):
eigs = roots(mat.charpoly(x=Dummy('x'), simplify=simplify), **flags)
else:
eigs = roots(mat.charpoly(x=Dummy('x'), **flags)
instead of one long line.
sympy/matrices/matrices.py
Outdated
| if isinstance(simplify, FunctionType): | ||
| eigs = roots(mat.charpoly(x=Dummy('x'), simplify=simplify), **flags) | ||
| else: | ||
| eigs = roots(mat.charpoly(x=Dummy('x'), simplify=_simplify), **flags) |
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.
We could leave simplify undefined. That would make it possible to change the default of charpoly with no modifications needed here.
sympy/matrices/matrices.py
Outdated
| # With 'multiple' flag set true, simplify() will be mapped for the list | ||
| # Otherwise, simplify() will be mapped for the keys of the dictionary | ||
| if not multiple: | ||
| return dict(map(lambda item: (simplify(item[0]), item[1]), eigs.items())) |
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.
Could this be something like: {simplify(key): value for key, value in eigs.items()}?
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.
@jksuom
Would there be any specific reason to use for loop
than using map iterator?
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 think that this is more readable. Also, the "for loop" is actually a dict comprehension which should be fairly efficient.
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.
@jksuom
If that is the case, I think another line below should also be changed into list comprehension.
How should I proceed?
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 personally prefer comprehensions but I know that map is used in many places in SymPy. In this case, I would accept either version.
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.
@jksuom
Okay, but I will just make it consistent with comprehensions at the moment.
I had originally had thought of coming up with an idea of using map, and how it can easily be implemented with multiprocessing.
But in this particular case, I am also seeing roots failing with some large scale matrices with random numerics, unable to provide benchmarks at the moment.
sympy/matrices/matrices.py
Outdated
| if not multiple: | ||
| return dict(map(lambda item: (simplify(item[0]), item[1]), eigs.items())) | ||
| else: | ||
| return list(map(lambda item: simplify(item), eigs)) |
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 think that this could be list(map(simplify, eigs)).
|
Thank you, this looks good. |
References to other Issues or PRs
#15118
Brief description of what is fixed or changed
I have found the line
sympy/sympy/matrices/matrices.py
Line 1143 in c6cf691
popping up the
'simplify'flag.Also, I had found that there was no procedure to handle to key, even if the key exists, in the function.
First I had defined new some variables to collect the keywords at the beginning of the procedure, to be able to use their informations regardless of any mutations in
**flags,and also changed the position of the flag collector
'multiple'below the line, to make it more explicitly noticeable.I could trace down that every procedure in the
eigenvals()function would return eigenvalues either as a list, or dictionary,So, secondly, I had created two separate procedures, to handle every possible cases of returns without any missing ones.
Any feedback would be welcomed.
Other comments
Release Notes
simplifyflag not working ineigenvals()