Vinbergs Algorithm #4152
Vinbergs Algorithm #4152
Conversation
simonbrandhorst
commented
Sep 26, 2024
•
simonbrandhorst
commented
- Add Vinberg's algorithm
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Simon Brandhorst <brandhorst@math.uni-sb.de>
Co-authored-by: Simon Brandhorst <brandhorst@math.uni-sb.de>
Co-authored-by: Simon Brandhorst <brandhorst@math.uni-sb.de>
Co-authored-by: Simon Brandhorst <brandhorst@math.uni-sb.de>
Co-authored-by: Simon Brandhorst <brandhorst@math.uni-sb.de>
| V = quadratic_space(QQ,Q) | ||
| diag, trafo = diagonal_with_transform(V) | ||
| @req count(is_positive, diag) == 1 "Q must be of signature (1,n)" | ||
| @req count(is_zero, diag) == 0 "Q must be of signature (1,n)" | ||
| @req count(is_negative, diag) > 0 "Q must be of signature (1,n)" | ||
| i = findfirst(is_positive, diag) | ||
| v0 = trafo[i:i,:] | ||
| v0 = ZZ.(denominator(v0)*v0) |
There was a problem hiding this comment.
@ElMerkel
here I moved from eigenvalues to to an implementation of Gram-Schmidt orthogonalisation since I am not convinced that Q is diagonalisable over the rationals.
| function _has_non_obtuse_angles!(tmp::ZZMatrix, Qv::ZZMatrix, roots::Vector{ZZMatrix}) | ||
| for r in roots | ||
| #tmp = r*Qv | ||
| mul!(tmp, r, Qv) | ||
| if !is_zero_entry(tmp,1, 1) && !is_positive_entry(tmp, 1, 1) | ||
| return false | ||
| end | ||
| end | ||
| return true | ||
| end |
There was a problem hiding this comment.
@ElMerkel
This is the non-allocating version I was referring to. Note however that using mul! is unsafe and can lead to segmentation faults if it gets bad input, e.g. matrices of a wrong size.
Hence it is only ever worth using in time critical places.
There was a problem hiding this comment.
Note that you can still make it perfectly safe by adding an input validation check yourself, i.e. in this case: that the number of rows resp. columns of the two matrices fit together.
src/NumberTheory/vinberg.jl
Outdated
| if v in roots | ||
| continue | ||
| end | ||
| v = _rescale_primitive(v) |
There was a problem hiding this comment.
@ElMerkel
it seems impossible to me that v is in roots already. After all (n,k) should be different.
... unless v is not primitive... but then this check is too late anyways.
|
For my part this is good to go. Thank you @ElMerkel . |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4152 +/- ##
========================================
Coverage 84.69% 84.69%
========================================
Files 627 628 +1
Lines 84301 84460 +159
========================================
+ Hits 71398 71534 +136
- Misses 12903 12926 +23
|
| @vprintln :Vinberg 1 "computing roots of squared length v^2=$(k) and v.v0 = $(n)" | ||
| possible_Vec = short_vectors_affine(Q, v0, QQ(n), k) | ||
| for v in possible_Vec | ||
| if !isone(reduce(gcd, v)) |
There was a problem hiding this comment.
I think this reduce(gcd, v) pops up multiple times here, and it could be made more efficient. Maybe move it into its own helper function?
Then you can make it potentially faster / allocate less by doing reduce(gcd!, v; init=one(v[1])).
Alas, that's not a requirement for this PR, just a suggestion for a future tweak.
* Add Vinberg's algorithm --------- Co-authored-by: Elias Merkel <elias.merkel@gmail.com> Co-authored-by: Max Horn <max@quendi.de> Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
