-
Notifications
You must be signed in to change notification settings - Fork 67
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
Random lines in LineEnumCtx
#1271
Conversation
Why |
I have not thought about this actually, I did not know it could make a difference. You mean for the operations in the |
@@ -16,7 +16,7 @@ function LineEnumCtx(K::T, n) where {T} | |||
depth = n + 1 | |||
dim = n | |||
q = order(K) | |||
length = divexact(q^n - 1, q - 1) | |||
length = divexact(BigInt(q)^n - 1, q - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
@@ -29,7 +29,7 @@ function LineEnumCtx(K::T, n::Int) where {T <: Union{fpField, FpField}} | |||
depth = n + 1 | |||
dim = n | |||
q = order(K) | |||
length = divexact(q^n - 1, q - 1) | |||
length = divexact(BigInt(q)^n - 1, q - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I cannot remove, otherwise all functions like collect
, or Vector{Int}(undef, length(P))
will not work if we set the length to be a ZZRingElem
src/QuadForm/LineOrbits.jl
Outdated
end | ||
p = size(K) | ||
j = i-2 | ||
n = findfirst(n -> sum(BigInt(p)^i for i in 1:n) > j, 1:256) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
src/QuadForm/LineOrbits.jl
Outdated
j = i-2 | ||
n = findfirst(n -> sum(BigInt(p)^i for i in 1:n) > j, 1:256) | ||
v[end-n] = one(K) | ||
j = n == 1 ? j : j-sum(BigInt(p)^k for k in 1:(n-1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here
test/QuadForm/LineOrbits.jl
Outdated
P = Hecke.enumerate_lines(GF(5), 20) | ||
io = IOBuffer() | ||
show(io, MIME"text/plain"(), P) | ||
@test length(String(take!(io))) == 98 | ||
show(IOContext(io, :supercompact => true), P) | ||
@test length(String(take!(io))) == 25 | ||
show(io, P) | ||
@test length(String(take!(io))) == 33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to increase test coverage, but maybe I can remove the test on the length of the strings and just leave the test calling the show functions (kind of tautological tests but it is "easy" test coverage for the corresponding lines)
src/QuadForm/Types.jl
Outdated
@@ -444,7 +444,7 @@ mutable struct LineEnumCtx{T, S} | |||
dim::Int | |||
depth::Int | |||
v::Vector{S} | |||
length::BigInt | |||
length::IntegerUnion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this means you just made anything use length
type unstable, unless you add a type assertion or a type conversion to every place that accesses length
. Perhaps that's OK, perhaps not -- you could avoid it by adding the type of length
as another type parameter to LineEnumCtx
.
Co-authored-by: Max Horn <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1271 +/- ##
==========================================
+ Coverage 74.41% 74.57% +0.16%
==========================================
Files 346 346
Lines 111321 111333 +12
==========================================
+ Hits 82836 83032 +196
+ Misses 28485 28301 -184
|
To prepare random Kneser's neighbour method, allow to have access to random lines from the
LineEnumCtx
. It might not be the most optimized way but at least it works.