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

Non-integral genera for ZLat #891

Merged
merged 7 commits into from Nov 29, 2022

Conversation

StevellM
Copy link
Collaborator

This is a first PR for a bigger import, namely to be able to have similar functionalities for GenusQuad and GenusHerm. According to the tests, the new changes do not break the previous results on integral lattices.

Note: I have added RationalUnion which behaves as IntegerUnion but we also get Rational{<: Integer} and fmpq with it. It allows one to have a common type for objects that can be coerced to fmpq.

There are some cleanups all the way to keep consistency and "file-style". In particular I have tried to replace all accessors by the appropriate (already defined) functions.

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Base: 73.70% // Head: 73.73% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (04cbabe) compared to base (4fa5133).
Patch coverage: 99.44% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #891      +/-   ##
==========================================
+ Coverage   73.70%   73.73%   +0.03%     
==========================================
  Files         345      345              
  Lines      100578   100684     +106     
==========================================
+ Hits        74128    74244     +116     
+ Misses      26450    26440      -10     
Impacted Files Coverage Δ
src/Hecke.jl 39.77% <ø> (ø)
src/QuadForm/Quad/ZGenus.jl 97.94% <99.38%> (+0.57%) ⬆️
src/QuadForm/Misc.jl 64.68% <100.00%> (+0.37%) ⬆️
src/QuadForm/Quad/ZLattices.jl 93.43% <100.00%> (-1.49%) ⬇️
src/Map/NumberField.jl 67.72% <0.00%> (-1.16%) ⬇️
src/NumFieldOrd/NfOrd/MaxOrd/Polygons.jl 88.66% <0.00%> (-0.79%) ⬇️
src/NumFieldOrd/NfOrd/Clgp/Saturate.jl 64.76% <0.00%> (-0.55%) ⬇️
src/NumFieldOrd/NfOrd/LLL.jl 77.34% <0.00%> (-0.45%) ⬇️
src/ModAlgAss/MeatAxe.jl 86.53% <0.00%> (-0.42%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@simonbrandhorst simonbrandhorst left a comment

Choose a reason for hiding this comment

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

the automorphous number stuff probably wont work for non-integral genera

src/QuadForm/Quad/ZGenus.jl Outdated Show resolved Hide resolved
src/QuadForm/Quad/ZGenus.jl Outdated Show resolved Hide resolved
rank = sig_pair[1] + sig_pair[2]
out = ZGenus[]
local_symbols = Vector{ZpGenus}[]
# every global genus has a 2-adic symbol
if mod(determinant, 2) == 1
push!(local_symbols, _local_genera(ZZ(2), rank, 0, 0, even))
if mod(numerator(determinant)*denominator(determinant), 2) == 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if mod(numerator(determinant)*denominator(determinant), 2) == 1
if mod(numerator(determinant),2)==1 && mod(denominator(determinant), 2) == 1

end
# collect the p-adic symbols
for p in prime_divisors(determinant)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you do not yet catch the example L = diag(1/3, 3) because det(L)=1. Instead you need the bad primes where the lattice is not unimodular. This is now given by the min and max scale.
Thus I think that your default arguments for the scale bounds are also questionable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So iterate over the bad primes instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just modified it so that we take in account also the primes for which the valuations of min_scale and max_scale are non trivial... As for the bounds on the scales, if the determinant is integral and min_scale is not specified, we get the same as before, if the determinant is strictly rational, then we adapt by imposing the maximal scale to be 1 if not specified (it is a choice to be consistent with what is happening on the other side). Of course we can change this "convention" but I guess it makes sense in general.

src/QuadForm/Quad/ZGenus.jl Outdated Show resolved Hide resolved
src/QuadForm/Quad/ZGenus.jl Show resolved Hide resolved
src/QuadForm/Quad/ZGenus.jl Outdated Show resolved Hide resolved
src/QuadForm/Quad/ZGenus.jl Outdated Show resolved Hide resolved
src/QuadForm/Quad/ZGenus.jl Show resolved Hide resolved
src/QuadForm/Quad/ZLattices.jl Outdated Show resolved Hide resolved
@StevellM
Copy link
Collaborator Author

For automorphous number stuff you mentioned, you mean that all functions having a ZGenus as input should check that the genus is actually integral?

@simonbrandhorst
Copy link
Collaborator

For automorphous number stuff you mentioned, you mean that all functions having a ZGenus as input should check that the genus is actually integral?

Probably it works even if it is not, but I did not check the details and so let us better stay on the safe side.

@simonbrandhorst simonbrandhorst merged commit aca916c into thofma:master Nov 29, 2022
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

2 participants