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.

Sign up for GitHub

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

Don't recompute congruence substitutions #240

Closed
wants to merge 1 commit into from
Closed

Don't recompute congruence substitutions #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions src/GenericCyclotomics.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,13 +365,6 @@ function (R::GenericCycloRing)(f::Dict{UPolyFrac,UPoly}; simplify::Bool=true) #
return GenericCyclo(f, R)
end

# congruence preparation
if R.congruence !== nothing
q = gen(base_ring(R), 1)
substitute = R.congruence[2] * q + R.congruence[1]
substitute_inv = (q - R.congruence[1]) * 1//R.congruence[2]
end

# reduce numerators modulo denominators
L = NTuple{4,UPoly}[]
for (g, c) in f
Copy link
Member

Choose a reason for hiding this comment

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

Style / taste question, but: I like to avoid nested control flow, esp. multi-level, to improve code clarity. So here I'd do

Suggested change
for (g, c) in f
for (g, c) in f
is_zero(c) && continue

and then drop the if !iszero(c) and thereby reduce the indention level inside the loop

Expand All @@ -381,8 +374,8 @@ function (R::GenericCycloRing)(f::Dict{UPolyFrac,UPoly}; simplify::Bool=true) #
else
gp =
evaluate(
numerator(g), [1], [substitute]
)//evaluate(denominator(g), [1], [substitute])
numerator(g), [1], [R.substitute]
)//evaluate(denominator(g), [1], [R.substitute])
Copy link
Member

Choose a reason for hiding this comment

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

If the fraction was reduced (i.e. numerator and denomiator are coprime) before the substitution, then it still is after, correct? But the // method can't know that and thus may end up trying to reduce? (I don't know if it actually does that, I just wonder).

In any case, you next need numerator and denominator anyway. So perhaps something like this could be used in this loop instead:

  den_g = denominator(g)
  num_g = numerator(g)
  if R.congruence === nothing
    den_g = evaluate(den_g, [1], [substitute])
    num_g = evaluate(num_g, [1], [substitute])
  end
  a, r = divrem(num_g, den_g)
  push!(L, (c, den_g, r, a))

This would arguably also make the code a little bit nicer?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the fraction was reduced (i.e. numerator and denomiator are coprime) before the substitution, then it still is after, correct?

No, not necessarily. This is kind of the whole point of this substitution. Take for example the fraction (q+1)//2. If q is now congruent to 1 modulo 2 then R.substitute is equal to 2*q+1. After evaluation the fraction is (2*q+2)//2 which is not reduced anymore. The idea is then that the fraction gets reduced to q+1 which has a normal form of 0. This detects that exp(2*pi*i*(q+1)//2) is equal to 1 for all q congruent to 1 modulo 2.

Copy link
Member

Choose a reason for hiding this comment

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

ah of course, gotcha.

So please add a comment explaining just that? :-)

end
a, r = divrem(numerator(gp), denominator(gp))
push!(L, (c, denominator(gp), r, a))
Expand Down Expand Up @@ -430,8 +423,8 @@ function (R::GenericCycloRing)(f::Dict{UPolyFrac,UPoly}; simplify::Bool=true) #
else
gp =
evaluate(
numerator(g), [1], [substitute_inv]
)//evaluate(denominator(g), [1], [substitute_inv])
numerator(g), [1], [R.substitute_inv]
)//evaluate(denominator(g), [1], [R.substitute_inv])
end
if haskey(fp, gp)
fp[gp] += cp * c
Expand Down Expand Up @@ -460,13 +453,11 @@ end

# Parent constructor

# TODO Maybe don't require at least one variable?
function generic_cyclotomic_ring(
R::UPolyRing;
congruence::Union{Tuple{ZZRingElem,ZZRingElem},Nothing}=nothing,
cached::Bool=true,
)
length(gens(R)) < 1 && error("At least one free variable is needed")
return GenericCycloRing(R, congruence)
end

Expand Down
17 changes: 17 additions & 0 deletions src/Types.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,23 @@ Generic cyclotomic ring
struct GenericCycloRing <: Ring
base_ring::UPolyRing
congruence::Union{Tuple{ZZRingElem,ZZRingElem},Nothing}
substitute::UPoly
substitute_inv::UPoly
function GenericCycloRing(
R::UPolyRing,
congruence::Union{Tuple{ZZRingElem,ZZRingElem},Nothing},
)
# TODO Maybe don't require at least one variable?
length(gens(R)) < 1 && error("At least one free variable is needed")
if congruence == nothing
return new(R, congruence)
else
q = gen(R, 1)
substitute = congruence[2] * q + congruence[1]
substitute_inv = (q - congruence[1]) * 1//congruence[2]
Copy link
Member

Choose a reason for hiding this comment

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

As discussed orally, computing this here in advance may be too costly. I would recommend changing the ring to a mutable struct (most of our ring types are anyway, to allow them to store attributes), and then leave the two fields undefined in the constructor.

Then add an accessor function (or two, depending on taste) to get their content, with suitable isdefined(R, :substitute) checks

Copy link
Member

@fingolfin fingolfin Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, why isn't this

Suggested change
substitute_inv = (q - congruence[1]) * 1//congruence[2]
substitute_inv = (q - congruence[1]) / congruence[2]

return new(R, congruence, substitute, substitute_inv)
end
end
end

@doc raw"""
Expand Down
Loading