-
Notifications
You must be signed in to change notification settings - Fork 59
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
Unify eigenvalue interface #1611
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1611 +/- ##
==========================================
+ Coverage 84.07% 84.10% +0.03%
==========================================
Files 93 93
Lines 36769 36826 +57
==========================================
+ Hits 30915 30974 +59
+ Misses 5854 5852 -2 ☔ View full report in Codecov by Sentry. |
Thanks for taking care of this. Two comments:
|
|
src/Aliases.jl
Outdated
@@ -8,6 +8,9 @@ include(joinpath(pathof(AbstractAlgebra), "..", "Aliases.jl")) | |||
@alias is_less isless | |||
@alias is_real isreal | |||
|
|||
@alias eigenvalues eigvals | |||
@alias eigenvalues_simple eigvals_simple |
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.
What does this function even do? I am asking as we didn't talk about it today.
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.
According to the doc string, it computes eigenvalues, but the function only works if the matrix has only "simple eigenvalues" (I assume simple = multiplicity 1).
Also allow matrices over rings
@@ -201,6 +201,8 @@ import AbstractAlgebra: set_attribute! | |||
|
|||
include("Exports.jl") | |||
|
|||
const eigenvalues = eigvals # alternative name for the function from LinearAlgebra |
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 didn't find a better place to put this. In Aliases.jl
it is too late.
This looks good now, and just waits for the next time we do a breaking release. |
This is one half of my proposal to unify the eigenvalue interface throughout OSCAR (oscar-system/Oscar.jl#2017). The other half is thofma/Hecke.jl#1344 .
The changes are:
eigvals
and introduce the aliaseigenvalues
(in consistency withLinearAlgebra
).factor(ZZ, QQ(2))
). I assume this change constitutes a breaking change.factor
, but not consistent witheigvals
fromLinearAlgebra
). Another breaking change.This is what seems to me to be "the most consistent" right now. I don't insist on any of these changes and I am happy to change things back/differently.
EDIT: This list is not up to date anymore. See oscar-system/Oscar.jl#2017 (comment) for what was decided.