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

Speed up polynomial mappings #4124

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 27 additions & 7 deletions src/Rings/MPolyMap/MPolyAnyMap.jl
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,37 @@ const _DomainTypes = Union{MPolyRing, MPolyQuoRing}
coeff_map::U
img_gens::Vector{V}
temp_ring # temporary ring used when evaluating maps
variable_indices::Vector{Int} # a table where the i-th entry contains the
# index of the variable where it's mapped to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# index of the variable where it's mapped to
# index of the variable where it is mapped to

# in case the mapping takes such a particularly
# simple form.

function MPolyAnyMap{D, C, U, V}(domain::D,
codomain::C,
coeff_map::U,
img_gens::Vector{V}) where {D, C, U, V}
@assert V === elem_type(C)
for g in img_gens
@assert parent(g) === codomain "elements does not have the correct parent"
end
codomain::C,
coeff_map::U,
img_gens::Vector{V}) where {D, C, U, V}
@assert V === elem_type(C)
for g in img_gens
@assert parent(g) === codomain "elements does not have the correct parent"
end
return new{D, C, U, V}(domain, codomain, coeff_map, img_gens)
end
function MPolyAnyMap{D, C, U, V}(domain::D,
codomain::C,
coeff_map::U,
img_gens::Vector{V}) where {D <: Union{<:MPolyRing, <:MPolyQuoRing},
C <: Union{<:MPolyRing, <:MPolyQuoRing},
U, V}
@assert V === elem_type(C)
for g in img_gens
@assert parent(g) === codomain "elements does not have the correct parent"
end
result = new{D, C, U, V}(domain, codomain, coeff_map, img_gens)
if all(is_gen(x) for x in img_gens) && allunique(img_gens)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if all(is_gen(x) for x in img_gens) && allunique(img_gens)
if all(is_gen, img_gens) && allunique(img_gens)

result.variable_indices = [findfirst(x==y for y in gens(codomain)) for x in img_gens]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result.variable_indices = [findfirst(x==y for y in gens(codomain)) for x in img_gens]
result.variable_indices = [findfirst(==(x), gens(codomain)) for x in img_gens]

end
return result
end
end

function MPolyAnyMap(d::D, c::C, cm::U, ig::Vector{V}) where {D, C, U, V}
Expand Down
35 changes: 35 additions & 0 deletions src/Rings/MPolyMap/MPolyRing.jl
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,47 @@ end
#
################################################################################

# Some additional methods needed for the test in the constructor for MPolyAnyMap
is_gen(x::MPolyQuoRingElem) = is_gen(lift(x))
is_gen(x::MPolyDecRingElem) = is_gen(forget_grading(x))
Comment on lines +136 to +137
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Memo to self: This is not legitimate. We need to switch to some internal method like _is_gen for that.


function _evaluate_plain(F::MPolyAnyMap{<:MPolyRing, <:MPolyRing}, u)
if isdefined(F, :variable_indices)
S = codomain(F)::MPolyRing
r = ngens(S)
ctx = MPolyBuildCtx(S)
for (c, e) in zip(AbstractAlgebra.coefficients(u), AbstractAlgebra.exponent_vectors(u))
ee = [0 for _ in 1:r]
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
ee = [0 for _ in 1:r]
ee = Vector{Int}(undef, r)

should be an epsilon faster

Copy link
Member

Choose a reason for hiding this comment

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

One should be able to use the "same" vector ee for the whole loop as it gets copied anyway. I try to optimize this further.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An undefined vector will not work. We need the zeroes.

for (i, k) in enumerate(e)
ee[F.variable_indices[i]] = k
end
push_term!(ctx, c, ee)
end
return finish(ctx)
end

return evaluate(u, F.img_gens)
end

function _evaluate_plain(F::MPolyAnyMap{<: MPolyRing}, u)
return evaluate(u, F.img_gens)
end

# See the comment in MPolyQuo.jl
function _evaluate_plain(F::MPolyAnyMap{<:MPolyRing, <:MPolyQuoRing}, u)
if isdefined(F, :variable_indices)
S = base_ring(codomain(F))::MPolyRing
r = ngens(S)
ctx = MPolyBuildCtx(S)
for (c, e) in zip(coefficients(u), exponents(u))
ee = [0 for _ in 1:r]
for (i, k) in enumerate(e)
ee[F.variable_indices[i]] = k
end
push_term!(ctx, c, ee)
end
return codomain(F)(finish(ctx))
end
A = codomain(F)
v = evaluate(lift(u), lift.(_images(F)))
return simplify(A(v))
Expand Down
Loading