-
-
Notifications
You must be signed in to change notification settings - Fork 205
Mpmath eigenvalues #1115
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
Mpmath eigenvalues #1115
Conversation
rocky
commented
Jan 17, 2021
We should add this option more pervasively. When converting from mpmath complex, convert to real number when imag is 0. We should also consider converting to int's too when possible.
| 'Eigenvalues[m_, OptionsPattern[Eigenvalues]]' | ||
|
|
||
| method = self.get_option(options, 'Method', evaluation) | ||
| if method and method.get_string_value() == "mpmath": |
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.
If you replace the string with a symbol, we should use method.has_form in here
| Compute Eigenvalues using mpmath's routines; Sympy is slow here and returns | ||
| complex numbers. | ||
| >> Eigenvalues[{{-8, 12, 4}, {12, -20, 0}, {4, 0, -2}}, Method->"mpmath"] |
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'd consider using a symbol to indicate the Method instead of a string (I think this is more consistent with the rest of the library, but I could be wrong). So I'd replace "mpmath" with MPMath.
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 got this idea from WL's Eigenvector. It uses strings for implementation options. I was thinking of "mpmath" more akin to "Arnoldi" ,"Banded", or "FEAST" rather than Automatic.
When I grep for "Method" right now in mathics/builtin, I am not coming up with any instances where there is a Symbol used, just strings:
image.py: >> Threshold[img, Method -> "Mean"]
image.py: >> Threshold[img, Method -> "Median"]
image.py: options = {"Method": '"Cluster"'}
lists.py: "bdmtd": 'Method in `` must be either "Optimize", "Agglomerate" or "KMeans".',
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 got this idea from WL's Eigenvector. It uses strings for implementation options. I was thinking of "mpmath" more akin to "Arnoldi" ,"Banded", or "FEAST" rather than Automatic.
When I grep for "Method" right now in mathics/builtin, I am not coming up with any instances where there is a Symbol used, just strings:
image.py: >> Threshold[img, Method -> "Mean"] image.py: >> Threshold[img, Method -> "Median"] image.py: options = {"Method": '"Cluster"'} lists.py: "bdmtd": 'Method in `` must be either "Optimize", "Agglomerate" or "KMeans".',
My bad, strings are the right choice then.
mathics/builtin/linalg.py
Outdated
| if matrix is None: | ||
| eigenvalues = ER.tolist() | ||
| # Sort the eigenvalues in the Mathematica convention: largest first. | ||
| eigenvalues.sort(key=lambda v: (abs(v[0]),-v[0].real,-(v[0].imag)), reverse=True) |
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 should probably extract this (the sorting routine) to a separate function to avoid duplication of code (so that we don't forget to update both lines if we want to update this in the future). This should be inside of a try block too.
|
Some thoughts:
|
|
Regarding what to use for Options (like |