Skip to content

Remove _map for universal polys #2206

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove _map for universal polys #2206

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9d393a
Remove `_map` for universal polys
SoongNoonien Nov 5, 2025
7506950
Upgrade if needed
SoongNoonien Nov 5, 2025
b193a53
Move variable check to `upgrade`
SoongNoonien Nov 6, 2025
2093761
Restore original kwarg name
SoongNoonien Nov 6, 2025
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
73 changes: 23 additions & 50 deletions src/generic/UnivPoly.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,9 @@ end
function set_exponent_vector!(p::UnivPoly, i::Int, exps::Vector{Int})
S = parent(p)
len = length(exps)
if len != nvars(parent(data(p)))
p.p = upgrade(S, data(p))
if len < nvars(S)
exps = vcat(exps, zeros(Int, nvars(S) - len))
end
p.p = upgrade(S, data(p))
if len < nvars(S)
exps = vcat(exps, zeros(Int, nvars(S) - len))
end
p.p = set_exponent_vector!(data(p), i, exps)
return p
Expand All @@ -99,11 +97,9 @@ function setcoeff!(p::UnivPoly, exps::Vector{Int}, c::T) where T <: RingElement
c = base_ring(data(p))(c)
S = parent(p)
len = length(exps)
if len != nvars(parent(data(p)))
p.p = upgrade(S, data(p))
if len < nvars(S)
exps = vcat(exps, zeros(Int, nvars(S) - len))
end
p.p = upgrade(S, data(p))
if len < nvars(S)
exps = vcat(exps, zeros(Int, nvars(S) - len))
end
p.p = setcoeff!(data(p), exps, c)
return p
Expand Down Expand Up @@ -620,15 +616,7 @@ function isless(a::UnivPoly{T}, b::UnivPoly{T}) where {T}
end
S = parent(a)
num = nvars(S)
Copy link
Member

@fingolfin fingolfin Nov 7, 2025

Choose a reason for hiding this comment

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

Suggested change
num = nvars(S)

s = data(a)
t = data(b)
if nvars(parent(s)) != num
s = upgrade(S, s)
end
if nvars(parent(t)) != num
t = upgrade(S, t)
end
return isless(s, t)
return isless(upgrade(S, data(a)), upgrade(S, data(b)))
Copy link
Member

@fingolfin fingolfin Nov 7, 2025

Choose a reason for hiding this comment

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

So we upgrade the underlying polynomials of a and b and then throw this away... I always wondered if that's a good idea. Well, it is certainly wise not to change this in this PR, but perhaps in a future PR we could think about introducing another helper (distinct from the one I suggest in another comment) like this:

function upgrade!(p::UniversalPolyRingElem)
  p.p = upgrade(parent(p), data(p))
  return p
end

and then here we can do

Suggested change
return isless(upgrade(S, data(a)), upgrade(S, data(b)))
return isless(upgrade!(a), upgrade!(b))

Copy link
Collaborator Author

@SoongNoonien SoongNoonien Nov 7, 2025

Choose a reason for hiding this comment

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

Yes, I thought about this too. I wasn't sure if it is a good idea that isless potentially modifies its inputs. Of course they would only be modified internally but shouldn't isless be named isless! then? (I'm not actually suggesting to rename isless, just wondering.)

Copy link
Member

@lgoettgens lgoettgens Nov 7, 2025

Choose a reason for hiding this comment

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

The internal representation may change at any point, as it is internal. We try to not change it during printing as that may lead to hard to track down bugs, but otherwise that is fine to do in a non-! function

Copy link
Collaborator Author

@SoongNoonien SoongNoonien Nov 7, 2025

Choose a reason for hiding this comment

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

Ok, alright then. I'll adjust the PR accordingly.

Copy link
Member

@lgoettgens lgoettgens Nov 7, 2025

Choose a reason for hiding this comment

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

Could you put this, and also the change that upgrade checks if it needs to do anything, into its own PR to not clutter this one here with mostly unrelated changes?

Copy link
Collaborator Author

@SoongNoonien SoongNoonien Nov 7, 2025

Choose a reason for hiding this comment

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

Yes, I can do this as well.

Copy link
Collaborator Author

@SoongNoonien SoongNoonien Nov 7, 2025

Choose a reason for hiding this comment

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

In which order would you like to have the changes?

Copy link
Member

@lgoettgens lgoettgens Nov 7, 2025

Choose a reason for hiding this comment

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

Maybe first do the other PR about upgrade, we merge that and then update the one here?

end

###############################################################################
Expand Down Expand Up @@ -673,10 +661,8 @@ function deflate(p::UnivPoly{T}, shift::Vector{Int}, defl::Vector{Int}) where {T
if vlen == num
return UnivPoly{T}(deflate(pp, shift, defl), S)
end
if vlen > num
pp = upgrade(S, pp)
num = nvars(parent(pp))
end
pp = upgrade(S, pp)
num = nvars(parent(pp))
if vlen < num
shift = vcat(shift, zeros(Int, num - vlen))
defl = vcat(defl, ones(Int, num - vlen))
Expand All @@ -697,10 +683,8 @@ function inflate(p::UnivPoly{T}, shift::Vector{Int}, defl::Vector{Int}) where {T
if vlen == num
return UnivPoly{T}(inflate(pp, shift, defl), S)
end
if vlen > num
pp = upgrade(S, pp)
num = nvars(parent(pp))
end
pp = upgrade(S, pp)
num = nvars(parent(pp))
if vlen < num
shift = vcat(shift, zeros(Int, num - vlen))
defl = vcat(defl, ones(Int, num - vlen))
Expand Down Expand Up @@ -958,8 +942,7 @@ end
_change_univ_poly_ring(R, Rx, cached::Bool) = universal_polynomial_ring(R, symbols(Rx); internal_ordering=internal_ordering(Rx), cached)[1]

function change_base_ring(R::Ring, p::UnivPoly{T}; cached::Bool=true, parent::UniversalPolyRing = _change_univ_poly_ring(R, parent(p), cached)) where {T <: RingElement}
base_ring(parent) != R && error("Base rings do not match.")
return _map(R, p, parent)
return UnivPoly(change_base_ring(R, upgrade(AbstractAlgebra.parent(p), data(p)); parent = mpoly_ring(parent)), parent)
Copy link
Member

@fingolfin fingolfin Nov 7, 2025

Choose a reason for hiding this comment

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

How about introducing a new helper, like this (untested code)

function upgrade(p::UniversalPolyRingElem)
  return upgrade(parent(p), data(p))
end

then calls to upgrade can further be simplified, e.g. here

Suggested change
return UnivPoly(change_base_ring(R, upgrade(AbstractAlgebra.parent(p), data(p)); parent = mpoly_ring(parent)), parent)
return UnivPoly(change_base_ring(R, upgrade(p); parent = mpoly_ring(parent)), parent)

Actually I still find this a bit difficult to untangle, splitting this up would help, e.g.

Suggested change
return UnivPoly(change_base_ring(R, upgrade(AbstractAlgebra.parent(p), data(p)); parent = mpoly_ring(parent)), parent)
p2 = upgrade(p)
p3 = change_base_ring(R, p2; parent = mpoly_ring(parent))
return UnivPoly(p3, parent)

and now I realize that there might be a problem, namely if nvars(mpoly_ring(parent)) and nvars(mpoly_ring(AA.parent(p))) differ (this is not a new problem of course, and perhaps best to ignore for now; my point is that the splitting enabled me to reason about the code in a way I couldn't when it was all in one line).

Copy link
Collaborator Author

@SoongNoonien SoongNoonien Nov 7, 2025

Choose a reason for hiding this comment

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

We can add this new upgrade method but #2198 changes this anyway.

end

function change_coefficient_ring(R::Ring, p::UnivPoly{T}; cached::Bool=true, parent::UniversalPolyRing = _change_univ_poly_ring(R, parent(p), cached)) where {T <: RingElement}
Expand All @@ -973,17 +956,7 @@ end
################################################################################

function map_coefficients(f::T, p::UnivPoly; cached::Bool=true, parent::UniversalPolyRing = _change_univ_poly_ring(parent(f(zero(base_ring(p)))), parent(p), cached)) where T
return _map(f, p, parent)
end

function _map(g::T, p::UnivPoly, Rx) where T
cvzip = zip(coefficients(p), exponent_vectors(p))
M = MPolyBuildCtx(Rx)
for (c, v) in cvzip
push_term!(M, g(c), v)
end

return finish(M)
return UnivPoly(map_coefficients(f, upgrade(AbstractAlgebra.parent(p), data(p)); parent = mpoly_ring(parent)), parent)
end

###############################################################################
Expand Down Expand Up @@ -1172,12 +1145,16 @@ end

function upgrade(S::UniversalPolyRing{T}, pp::MPolyRingElem{T}) where {T}
n = nvars(S) - nvars(parent(pp))
ctx = MPolyBuildCtx(mpoly_ring(S))
v0 = zeros(Int, n)
for (c, v) in zip(coefficients(pp), exponent_vectors(pp))
push_term!(ctx, c, vcat(v, v0))
if n > 0
ctx = MPolyBuildCtx(mpoly_ring(S))
v0 = zeros(Int, n)
for (c, v) in zip(coefficients(pp), exponent_vectors(pp))
push_term!(ctx, c, vcat(v, v0))
end
return finish(ctx)
else
return pp
end
return finish(ctx)
end

function (a::UniversalPolyRing{T})(b::RingElement) where {T <: RingElement}
Expand All @@ -1198,11 +1175,7 @@ end

function (S::UniversalPolyRing{T})(p::UnivPoly{T}) where {T <: RingElement}
parent(p) !== S && error("Unable to coerce")
n = nvars(S) - nvars(parent(data(p)))
if n != 0
p = UnivPoly{T}(upgrade(S, data(p)), S)
end
return p
return UnivPoly{T}(upgrade(S, data(p)), S)
end

function (a::UniversalPolyRing{T})(b::Vector{T}, m::Vector{Vector{Int}}) where {T <: RingElement}
Expand Down
Loading