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

[QuadFormAndIsom]: new fix, cleanup, additional improvements and better documentation #4546

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

StevellM
Copy link
Member

@StevellM StevellM commented Feb 6, 2025

  • Cleanup of the code to avoid redundant procedures, fix some typos and make sure to fit into the suggested guideline;
  • Fix the function _action_on_genus which was not working as wanted for dyadic ramified primes;
  • Add a new feature: equivariant_primitive_extensions now make use of the minimal polynomial of the isometries to be extended. This simplify the search of gluing domains, over which isometries should coincide;
  • Improve a bit the documentation to reference some more general methods;
  • Turn some more @req into return statement, with trivial outputs.

Then further improvements when possible, to try to reduce the number of allocations and avoid redundant computations.

…+ fix galois action + add use minpoly for equivariant primitive extensions
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 96.03524% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.45%. Comparing base (0d59f06) to head (022f844).
Report is 37 commits behind head on master.

Files with missing lines Patch % Lines
experimental/QuadFormAndIsom/src/embeddings.jl 95.58% 6 Missing ⚠️
experimental/QuadFormAndIsom/src/enumeration.jl 93.33% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4546      +/-   ##
==========================================
- Coverage   84.55%   84.45%   -0.10%     
==========================================
  Files         672      672              
  Lines       88833    89215     +382     
==========================================
+ Hits        75109    75350     +241     
- Misses      13724    13865     +141     
Files with missing lines Coverage Δ
...ntal/QuadFormAndIsom/src/lattices_with_isometry.jl 99.44% <100.00%> (ø)
...mental/QuadFormAndIsom/src/spaces_with_isometry.jl 98.48% <100.00%> (ø)
experimental/QuadFormAndIsom/src/types.jl 100.00% <ø> (ø)
experimental/QuadFormAndIsom/test/runtests.jl 100.00% <100.00%> (ø)
experimental/QuadFormAndIsom/src/enumeration.jl 94.03% <93.33%> (+0.35%) ⬆️
experimental/QuadFormAndIsom/src/embeddings.jl 96.52% <95.58%> (-0.10%) ⬇️

... and 37 files with indirect coverage changes

Comment on lines +473 to +483
_s = sym.data
# sym.data is always a list of triples, but we also need the norm valuations
# whenever sym is dyadic and ramified. For those cases, we add it to our local symbol.
if is_dyadic(sym) && is_ramified(sym)
nn = sym.norm_val
@assert length(nn) == length(_s)
s = Tuple{Int, Int, Int, Int}[(_s[i][1], _s[i][2], _s[i][3], nn[i]) for i in 1:length(nn)]
push!(lgs_new, genus(HermLat, E, t(sym.p), s))
else
push!(lgs_new, genus(HermLat, E, t(sym.p), _s))
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the fix for _action_on_genus

@StevellM StevellM added experimental Only changes experimental parts of the code release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Feb 6, 2025
z = zero_matrix(QQ, 0, degree(cover(D)))
glue = reduce(vcat, QQMatrix[matrix(QQ, 1, degree(cover(D)), g) for g in _glue]; init=z)
glue = vcat(bAB, glue)
Fakeglue = Hecke.FakeFmpqMat(glue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe there are some ongoing efforts on getting rid of FakeFmpqMat

@simonbrandhorst simonbrandhorst merged commit edbdb5f into oscar-system:master Feb 7, 2025
29 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Only changes experimental parts of the code release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants