Quadratic forms and isometries - an Oscar (experimental) project #2563
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2563 +/- ##
==========================================
- Coverage 72.84% 72.32% -0.52%
==========================================
Files 416 427 +11
Lines 55600 59893 +4293
==========================================
+ Hits 40503 43319 +2816
- Misses 15097 16574 +1477
|
There was a problem hiding this comment.
I do not have any knowledge about the mathematics behind all of this. But I have some minor remarks and nitpicks about the style and readability (see below). Feel free to ignore them.
And I think that your code is not formatted according to the .editorconfig and .JuliaFormatter.toml. It would be great if you could adapt that.
| @@ -0,0 +1,13 @@ | |||
| function set_lwi_level(k::Int) | |||
There was a problem hiding this comment.
Is this lwi here an abbreviation of something? Maybe change it to a more meaningful name
There was a problem hiding this comment.
It is not meant to stay so forever, but only during the "experimental part" of this code. lwi means "lattice with isometry", which was the original name of the project. I would rather keep it like that because it is short enough, and the meaning is not really important compared to what it is used for (the documentation currently covers the use of the function.
|
Thanks @lgoettgens for the comments, I will make some changes accordingly. |
|
The julia 1.6 failure (https://github.com/oscar-system/Oscar.jl/actions/runs/5596410493/jobs/10233396520?pr=2563) is #2570. On julia 1.9 and 1.10, "Enumeration of lattices with finite isometries" test needs 6min30s and 9min, while on nightly it gets killed after 90min. |
|
Yes, we have two problems here I guess: first the codes are calling functions in Hecke which are not precompiled by default. Second is that to have relevant tests, which covers most of the feature, I need to resort to some expensive algorithms. I could reduce them but then we loose of the coverage of the codes in case of breaking changes. |
|
Do things look good now ? @lgoettgens @thofma If yes, it is good to go for me. |
|
My comments seem to be all resolved now. But as wrote already, I have no knowledge about the mathematics behind, thus I will not approve this pr and leave that to e.g. @thofma |
| # Quadratic forms and isometries | ||
|
|
||
| This project is a complement to the code about *hermitian lattices* available | ||
| on Hecke. We aim here to connect Hecke and GAP to handle some algorithmic |
There was a problem hiding this comment.
| on Hecke. We aim here to connect Hecke and GAP to handle some algorithmic | |
| in Hecke. We aim here to connect Hecke and GAP to handle some algorithmic |
| This project is a complement to the code about *hermitian lattices* available | ||
| on Hecke. We aim here to connect Hecke and GAP to handle some algorithmic | ||
| methods regarding quadratic forms with their isometries. In particular, | ||
| the integration of this code to Oscar is necessary to benefit all the |
There was a problem hiding this comment.
| the integration of this code to Oscar is necessary to benefit all the | |
| the integration of this code within Oscar is necessary to benefit from all the |
| The former parametrizes pairs $(V, f)$ where $V$ is a rational quadratic form | ||
| and $f$ is an isometry of $V$. The latter parametrizes pairs $(L, f)$ where | ||
| $L$ is an integral quadratic form, also known as $\mathbb Z$-lattice and $f$ | ||
| is an isometry of $L$. One of the main feature of this project is the |
There was a problem hiding this comment.
| is an isometry of $L$. One of the main feature of this project is the | |
| is an isometry of $L$. One of the main features of this project is the |
| and $f$ is an isometry of $V$. The latter parametrizes pairs $(L, f)$ where | ||
| $L$ is an integral quadratic form, also known as $\mathbb Z$-lattice and $f$ | ||
| is an isometry of $L$. One of the main feature of this project is the | ||
| enumeration of isomorphism classes of pairs $(L, f)$ where $f$ is an isometry |
There was a problem hiding this comment.
| enumeration of isomorphism classes of pairs $(L, f)$ where $f$ is an isometry | |
| enumeration of isomorphism classes of pairs $(L, f)$, where $f$ is an isometry |
|
|
||
| This project has been slightly tested on simple and known examples. It is | ||
| currently being tested on a larger scale to test its reliability. Moreover, | ||
| there are still computational bottlenecks due to non optimized algorithms. |
There was a problem hiding this comment.
| there are still computational bottlenecks due to non optimized algorithms. | |
| there are still computational bottlenecks due to non-optimized algorithms. |
| true | ||
| ``` | ||
| """ | ||
| rank(Vf::QuadSpaceWithIsom) = rank(space(Vf))::Integer |
There was a problem hiding this comment.
| rank(Vf::QuadSpaceWithIsom) = rank(space(Vf))::Integer | |
| rank(Vf::QuadSpaceWithIsom) = rank(space(Vf)) |
Should not be necessary?
| true | ||
| ``` | ||
| """ | ||
| dim(Vf::QuadSpaceWithIsom) = dim(space(Vf))::Integer |
There was a problem hiding this comment.
| dim(Vf::QuadSpaceWithIsom) = dim(space(Vf))::Integer | |
| dim(Vf::QuadSpaceWithIsom) = dim(space(Vf)) |
| end | ||
|
|
||
| @doc raw""" | ||
| Base.:^(Vf::QuadSpaceWithIsom, n::Int) |
There was a problem hiding this comment.
| Base.:^(Vf::QuadSpaceWithIsom, n::Int) | |
| ^(Vf::QuadSpaceWithIsom, n::Int) -> QuadSpaceWithIsom |
src/Groups/spinor_norms.jl
Outdated
| @@ -478,7 +478,7 @@ julia> order(Oq) | |||
| # we can compute the orthogonal group of L | |||
| Oq = orthogonal_group(discriminant_group(L)) | |||
| G = orthogonal_group(L) | |||
| return sub(Oq, [Oq(g, check=false) for g in gens(G)]) | |||
| return sub(Oq, unique([Oq(g, check=false) for g in gens(G)])) | |||
There was a problem hiding this comment.
| return sub(Oq, unique([Oq(g, check=false) for g in gens(G)])) | |
| return sub(Oq, unique!([Oq(g, check=false) for g in gens(G)])) |
| true | ||
| ``` | ||
| """ | ||
| gram_matrix(Vf::QuadSpaceWithIsom) = gram_matrix(space(Vf))::QQMatrix |
There was a problem hiding this comment.
Why all those annotations?
There was a problem hiding this comment.
Isn't it better for type stability ?
There was a problem hiding this comment.
Hm, it should not matter, since gram_matrix of a quadratic space should be type stable.
|
Doctests are suddenly failing? |
|
Yes it appears that my fix was not a fix... I am working on it. |
Here is (finally) the first PR of the project on integral/rational quadratic forms and their isometries. It will stay as a draft for now because I need to check code coverage and deployment of the html documentation.
As a further remark: we cannot put all relevant tests for the project on Oscar since they are too heavy. I have put some tests for now, quite small and I might have to reduce them (at the end they will be "tautological" but necessary to ensure that the codes run, at least)
We are working on getting to heavier tests of all the functionalities, but we need more improvements on some other Oscar/Hecke features to do this. The project would be ready to be made available in the
srcof Oscar as soon as those heavy checks would have been performed (and with the agreement of the maintainers).For the curious reviewers here: this project covers the algorithms present in the paper "Finite subgroups of automorphisms of K3 surfaces" of Simon Brandhorst and Tommy Hofmann. In particular, have been implemented the enumeration techniques for isometries with at most 2 prime divisors on even integer lattices, and the hermitian version of what they call "Miranda-Morrison theory". Moreover, we provide a first implementation of Nikulin's theory on primitive embeddings of even integer lattices in the primary case - more cases should be available in the future, for research reason or on demand of some users (maybe some more before the end of my PhD).
@thofma @simonbrandhorst here it is