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

Add is_nilpotent(::MatrixElem) #1783

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Conversation

lgoettgens
Copy link
Collaborator

I need something for checking nilpotency of a matrix in OSCAR. If you have good ideas for speed improvements, please let me know. Otherwise, I would be happy to get this merged soonish, and performance things can be adapted later anyway.

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.04%. Comparing base (025eabc) to head (1bbf5b6).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1783   +/-   ##
=======================================
  Coverage   88.03%   88.04%           
=======================================
  Files         117      117           
  Lines       30088    30102   +14     
=======================================
+ Hits        26489    26503   +14     
  Misses       3599     3599           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@thofma thofma left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Maybe @fieker wants to comment/approve?

is_zero(A) && return true
while i < n
i *= 2
A = mul!(A, A, A)
Copy link
Member

Choose a reason for hiding this comment

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

It might or might not be faster to compute the minimal or characteristic polynomial (if there are dedicated optimized methods for this and the given matrix type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's true. but for that one would need to know which specialized methods exist in which cases, which I honestly don't

A = deepcopy(A)
i = 1
is_zero(A) && return true
while i < n
Copy link
Member

Choose a reason for hiding this comment

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

Actually, is this algorithm even correct when A contains zero divisors? I normally think about matrices over integral domains, and then of course A is nilpotent iff A^n=0. But over a ring with zero-divisors, we could have a triangularizable matrix with nilpotent eigenvalues where $A^n\neq0$ but still $A^k=0$ for some $k&gt;n$.

Admittedly this is somewhat fringe, but we should still handle it right? For starters we could add is_domain_type(T) || error("Only supported over integral domains") ? (AFAIK there is no is_domain / is_integral_domain which for e.g. $\mathbb{Z}/n\mathbb{Z}$ takes the modulus $n$ into account)

Then if someone needs this over non-domains, they at least get a helpful error and can provide an implementation for their use case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Thanks for mentioning this. I added the suggested error

@lgoettgens
Copy link
Collaborator Author

The HeckeCI failures are thofma/Hecke.jl#1600.

@lgoettgens
Copy link
Collaborator Author

The OscarCI job fails due to the same problem in the Hecke doctests (why do they even run in this job? I thought that's why we have the HeckeCI job).
Can someone force-merge this?

@lgoettgens lgoettgens merged commit cfdcd27 into Nemocas:master Sep 11, 2024
30 checks passed
@lgoettgens lgoettgens deleted the lg/is_nilpotent branch September 11, 2024 06:52
@lgoettgens
Copy link
Collaborator Author

The OscarCI job fails due to the same problem in the Hecke doctests (why do they even run in this job? I thought that's why we have the HeckeCI job). Can someone force-merge this?

I reported the issue with the OscarCI job running the Hecke doctests in oscar-system/OscarDevTools.jl#38.

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

Successfully merging this pull request may close these issues.

3 participants