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

QuadFormWithIsom: further cleanups + preparation for incoming new features #2847

Merged

Conversation

StevellM
Copy link
Member

Here are some further minor changes for the package:

  • Clean-up of the docstrings and of the html documentation (I was not happy with how it looked);

    • Fix the different bugs in display, the typos and other mistakes
    • Add more internal references to different part of the html documentation
    • Add more descriptions/comments for the internal functions
  • More improvements following the previous patch:

    • Disable more unnecessary checks
    • Avoid the creation of some lists in largely used functions when it can be replace by "smarter math"
    • Avoid to use cartesian_product_iterator for products of two lists, and put back inplace = true for products of multiple lists (we might save more time and memory now)
    • Rename some variables which should not have the same type in a same code
  • Few more functionalities for the soon to be added complement on equivariant primitive embeddings:

    • Recognition of primary, elementary and unimodular lattices with isometry
    • Computation of orthogonal submodules (whenever it makes sense)
    • Computation of the representation of a group of isometries for a lattice on the discriminant group of that lattice
    • image_centralizer_in_Oq extended to the general case using hermitian Miranda-Morrison and gluing of stabilizers along equivariant primitive extensions
    • Make the function primitive_extensions part of the available functions and not only a background routine (to be safe I have added a sort of deprecations)

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Merging #2847 (593d3bc) into master (aa8e64f) will decrease coverage by 0.06%.
Report is 26 commits behind head on master.
The diff coverage is 97.92%.

@@            Coverage Diff             @@
##           master    #2847      +/-   ##
==========================================
- Coverage   80.65%   80.60%   -0.06%     
==========================================
  Files         456      456              
  Lines       64729    65411     +682     
==========================================
+ Hits        52209    52723     +514     
- Misses      12520    12688     +168     
Files Coverage Δ
experimental/QuadFormAndIsom/src/enumeration.jl 96.02% <100.00%> (+<0.01%) ⬆️
...mental/QuadFormAndIsom/src/spaces_with_isometry.jl 98.36% <ø> (ø)
experimental/QuadFormAndIsom/src/types.jl 100.00% <ø> (ø)
experimental/QuadFormAndIsom/test/runtests.jl 97.29% <ø> (ø)
...ntal/QuadFormAndIsom/src/lattices_with_isometry.jl 98.94% <98.23%> (+2.34%) ⬆️
experimental/QuadFormAndIsom/src/embeddings.jl 98.04% <97.53%> (+0.16%) ⬆️

... and 56 files with indirect coverage changes

Co-authored-by: Max Horn <max@quendi.de>
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me. Perhaps @simonbrandhorst should also approve and/or merge it?

Comment on lines 611 to 612
Indeed, by fixing $H$, $M$ and $N$, one fixes a unique genus $G$ of even
primitive extensions of $M\oplus N$ with a glue map described as above. We can
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure this statement is true. Can you give a reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

I might be wrong but I guess this follows from Nikulin. If you have fixed isometry class of subgroups $H_M$ for $D_M$ and $H_N$ for $D_N$ which are anti-isometric, then the isometry class of the discriminant form for the overlattice is independent on the choice of the representatives in the isometry classes of $H_M$ and $H_N$. Or am I using a wrong shortcut ? Otherwise I could just revise the documentation to avoid this statement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed, this part of the code needs to be changed to be correct.

experimental/QuadFormAndIsom/src/lattices_with_isometry.jl Outdated Show resolved Hide resolved
experimental/QuadFormAndIsom/src/lattices_with_isometry.jl Outdated Show resolved Hide resolved
@StevellM
Copy link
Member Author

There is more than what was originally planned: we remarked that the code for primitive_embeddings was not totally right for some cases that were not reached. It has been modified accordingly.

@fingolfin
Copy link
Member

What's the status here? Is this ready now, or is more work needed?

@StevellM
Copy link
Member Author

StevellM commented Oct 5, 2023

For me it is good to go, maybe @simonbrandhorst would like to check whether my fixes are correct.

@simonbrandhorst simonbrandhorst merged commit 588abce into oscar-system:master Oct 13, 2023
11 of 15 checks passed
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.

None yet

3 participants