Introduce universal ring by SoongNoonien · Pull Request #2274 · Nemocas/AbstractAlgebra.jl

SoongNoonien · 2026-01-07T15:17:40Z

This finally introduces the universal ring (#2172). Ideally, I'd like to get this into 0.48. I'm not sure if the hashing is what we want. There is no documentation on what a ring should implement to be a base ring of a universal ring and there are currently no tests which test this for other rings besides multivariate polynomial rings.

Let's see what the tests say, the doctest will fail as I haven't adjusted them yet.

SoongNoonien · 2026-01-07T15:47:05Z

Ok, caching makes some problems. The caching now depends on the base ring which is created with cached = false by default. Is this what we want?

SoongNoonien · 2026-01-08T15:59:10Z

The remaining doctest failure is caused by the changed caching. We could adjust the universal_polynomial_ring constructor to only return the variables specified while constructing, even if there already are more variables. But then we should perhaps even change what number_of_variables does for a universal ring. Should it return Inf?

lgoettgens · 2026-01-08T16:11:13Z

The remaining doctest failure is caused by the changed caching. We could adjust the universal_polynomial_ring constructor to only return the variables specified while constructing, even if there already are more variables. But then we should perhaps even change what number_of_variables does for a universal ring. Should it return Inf?

This is even worse than you describe. Consider the following:

julia> using AbstractAlgebra # aka fresh session

julia> universal_polynomial_ring(ZZ, [:x])
(Universal Polynomial Ring over Integers, AbstractAlgebra.Generic.UniversalRingElem{AbstractAlgebra.Generic.MPoly{BigInt}}[x])

julia> universal_polynomial_ring(ZZ, [:y])
(Universal Polynomial Ring over Integers, AbstractAlgebra.Generic.UniversalRingElem{AbstractAlgebra.Generic.MPoly{BigInt}}[x])

when I request a univ poly ring with variable y, I get one in variable x. This is a problem, which could be fixed by making the variables passed to the constructor be part of the cache key

SoongNoonien · 2026-01-08T16:17:16Z

This is a problem, which could be fixed by making the variables passed to the constructor be part of the cache key

Yes, this is true but ideally the caching shouldn't depend on the arbitrary variables passed on creation.

lgoettgens · 2026-01-08T16:20:21Z

This is a problem, which could be fixed by making the variables passed to the constructor be part of the cache key

Yes, this is true but ideally the caching shouldn't depend on the arbitrary variables passed on creation.

This was requested in #1993 (comment) (apparently by @thofma, but I don't find the comment; just the one where @fingolfin suggests this)

Edit: found it: #1989 (comment)

fingolfin · 2026-01-08T16:49:22Z

I specifically suggested to remove s (the list of variables) from the cache key when we talked about this earlier.

My reasoning for the removal was/is this: the primary reason we introduced universal polynomial rings in the first place was to accommodate users who are more used to systems like Mathematica and Maple, were you don't have to "define" variables or rings; if you need a new variable x, then you simply write x and be done. We can't quite reach that, but the universal polynomial ring is close to that.

For this kind of users, we also want caching. So that they don't want have to worry why there are multiple "different" x...

But if one gets different universal polynomials just depending on what the initial variables used where, that's defeating all of this.

Of course this is bad:

julia> using AbstractAlgebra # aka fresh session

julia> universal_polynomial_ring(ZZ, [:x])
(Universal Polynomial Ring over Integers, AbstractAlgebra.Generic.UniversalRingElem{AbstractAlgebra.Generic.MPoly{BigInt}}[x])

julia> universal_polynomial_ring(ZZ, [:y])
(Universal Polynomial Ring over Integers, AbstractAlgebra.Generic.UniversalRingElem{AbstractAlgebra.Generic.MPoly{BigInt}}[x])

But here the fix should be to adjust the constructor: change

function universal_polynomial_ring(R::Ring, varnames::Vector{Symbol}; cached::Bool=true, internal_ordering::Symbol=:lex)
   @req !is_trivial(R) "Zero rings are currently not supported as coefficient ring."
   T = elem_type(R)
   U = mpoly_type(R)

   S = Generic.UniversalPolyRing{T}(R, varnames, internal_ordering, cached)
   return (S, gens(S))
end

by replacing the return with

   return (S, gens(S, varnames))

fingolfin · 2026-01-08T16:50:31Z

(That said: if this is controversial (it seems to be; I totally forgot about those past conversations Lars dug up), then we can of course also restore the use of s as cache key, and discuss its pros and cons in a future PR. I don't mean to sneak it in or rush it through.

fingolfin · 2026-01-08T19:47:04Z

 ###############################################################################

-function content(a::UniversalPolyRingElem)
+function content(a::UniversalRingElem{<:MPolyRingElem})


Is this really necessary given the definition of UniversalPolyRingElem above?

No, it is not. But I thought that it would be better to not use the aliases where possible. Ideally they should just be there for increased backwards compatibility.

Hmm, I think there is a lot of merit in keeping UniversalPolyRingElem around: it is easier to grasp and less technical than UniversalRingElem{<:MPolyRingElem}.

Plus, not changing it in this PR makes reviewing this PR that much easier, as I don't have to look over all those bits that don't really change anything ;-)

I forgot about this comment... @lgoettgens suggested to move the aliases to src/Deprecations.jl, which is what I did now. Is this fine by you @fingolfin or do you still think that we should keep them?

To keep this discussion here understandable on its own, let me repeat my argument. UniversalPolyRingElem{T} for a concrete T used to be a concrete type, but will not be concrete after this PR. Therefore, it should not be used as the eltype of a Vector etc. For places like here that just use it together with <: they would be fine to keep using. But I am not quite sure how to explain that in the docs and would thus just discourage use of the aliases altogether

SoongNoonien · 2026-01-13T12:45:34Z

I don't get this error message. Locally the respective line runs fine.

codecov · 2026-01-15T12:27:25Z

Codecov Report

❌ Patch coverage is 86.55172% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.13%. Comparing base (b873b3d) to head (89729b2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/UniversalRing.jl	77.77%	50 Missing ⚠️
src/UnivPoly.jl	91.48%	28 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2274      +/-   ##
==========================================
+ Coverage   88.12%   88.13%   +0.01%     
==========================================
  Files         128      130       +2     
  Lines       32870    32871       +1     
==========================================
+ Hits        28966    28971       +5     
+ Misses       3904     3900       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lgoettgens · 2026-03-18T14:22:19Z

This PR has conflicts

SoongNoonien · 2026-03-18T14:42:22Z

Not anymore ;-)

lgoettgens · 2026-03-18T14:30:59Z

IMO this file could be merged into src/UniversalRing.jl to have the code less splattered around

I tried that before but I had some problems which forced me somehow to export and import stuff back and forth between AbstractAlgebra.Generic and AbstractAlgebra.

lgoettgens · 2026-03-18T14:51:16Z

+zero(R::UniversalRing{T, U}) where {T, U} = UniversalRingElem{T, U}(zero(base_ring(R)), R)
+
+one(R::UniversalRing{T, U}) where {T, U} = UniversalRingElem{T, U}(one(base_ring(R)), R)


Suggested change

zero(R::UniversalRing{T, U}) where {T, U} = UniversalRingElem{T, U}(zero(base_ring(R)), R)

one(R::UniversalRing{T, U}) where {T, U} = UniversalRingElem{T, U}(one(base_ring(R)), R)

zero(R::UniversalRing{T, U}) where {T, U} = R(zero(base_ring(R)))

one(R::UniversalRing{T, U}) where {T, U} = R(one(base_ring(R)))

this should be equivalent but IMO better readable

Yes, this should be equivalent but this involves an additional coercion check. Shouldn't it be possible to drop the type parameters as well, if we were to do it like you proposed?

lgoettgens · 2026-03-18T14:54:55Z

+   if length(a) != length(b)
+      return false
+   end
+   n1 = nvars(parent(data(a)))


why not upgrade here, as in isless below?

I don't know apparently this is supposed to be faster.

The fallback is implemented with an upgrade.

SoongNoonien commented Jan 7, 2026

View reviewed changes

Comment thread src/UnivPoly.jl Outdated

SoongNoonien force-pushed the univ_ring_test branch from e5d56cc to 9d32f31 Compare January 7, 2026 15:32

lgoettgens added the breaks printing label Jan 8, 2026

lgoettgens reviewed Jan 8, 2026

View reviewed changes

Comment thread src/generic/GenericTypes.jl Outdated

fingolfin reviewed Jan 8, 2026

View reviewed changes

lgoettgens reviewed Jan 8, 2026

View reviewed changes

Comment thread src/generic/GenericTypes.jl Outdated

lgoettgens reviewed Jan 8, 2026

View reviewed changes

Comment thread src/generic/UnivPoly.jl Outdated

fingolfin reviewed Jan 8, 2026

View reviewed changes

Comment thread docs/src/univpolynomial.md Outdated

SoongNoonien force-pushed the univ_ring_test branch 4 times, most recently from 25dd4d6 to 13e8404 Compare January 13, 2026 11:12

SoongNoonien added the triage label Jan 13, 2026

fingolfin removed the triage label Jan 14, 2026

This comment was marked as resolved.

Sign in to view

SoongNoonien mentioned this pull request Jan 16, 2026

Drop some AbstractAlgebra.Generic. in action poly code oscar-system/Oscar.jl#5705

Open

fingolfin reviewed Jan 17, 2026

View reviewed changes

Comment thread src/generic/UnivPoly.jl Outdated

fingolfin reviewed Jan 17, 2026

View reviewed changes

Comment thread src/generic/UnivPoly.jl Outdated

Comment thread src/generic/UnivPoly.jl Outdated

Comment thread src/generic/UnivPoly.jl Outdated

Comment thread src/UnivPoly.jl Outdated

Comment thread src/generic/UnivPoly.jl Outdated

Comment thread src/generic/UnivPoly.jl Outdated

SoongNoonien force-pushed the univ_ring_test branch 3 times, most recently from d2069f5 to bb2ed4f Compare January 21, 2026 11:46

SoongNoonien added the breaking label Feb 25, 2026

SoongNoonien added 10 commits March 18, 2026 15:39

Introduce universal ring

e1a0da7

UniversalRing: Merge abstract and concrete types

697f7ec

UniversalRing: Simplify some type specifications

bb1c713

Add stable hash for universal polynomials

fcd9df6

UniversalRing: Add second type parameter

942bf7a

Move universal polynomial code to another file

4b563fb

Add universal laurent polynomials

959cd6d

UniversalRing: Add basic documentation

214e01a

Don't mix universal polynomial types

b91f7e1

UniversalRing: Adjust type aliases

30117fc

SoongNoonien force-pushed the univ_ring_test branch from 44184a7 to 30117fc Compare March 18, 2026 14:41

lgoettgens reviewed Mar 18, 2026

View reviewed changes

SoongNoonien added 10 commits March 20, 2026 11:35

Fix some typos

76d3352

Add documentation for cached in universal ring constructors

e5b9aa3

Avoid base_ring(base_ring()) in universal ring code

ace961b

Use upgrade! in univ_promote!

64b553d

Rename data field of universal ring elements

11287c5

Fix fallback hash for universal ring elements

daa58e5

Move type aliases to Deprecations.jl

01b7928

Simplify UniversalRing constructor

a308efa

Fix typos in universal_laurent_polynomial_ring docstring

348c75d

Fix UnivPoly tests

89729b2

SoongNoonien mentioned this pull request Mar 20, 2026

Avoid access to p field of universal ring elements oscar-system/Oscar.jl#5885

Merged

SoongNoonien referenced this pull request in oscar-system/GenericCharacterTables.jl Mar 20, 2026

Version 0.8.1 (#318)

1a2af4e

SoongNoonien mentioned this pull request Mar 20, 2026

Use GenericCharacterTables 0.8.1 for book tests oscar-system/Oscar.jl#5886

Merged

SoongNoonien closed this Mar 23, 2026

SoongNoonien reopened this Mar 23, 2026

		zero(R::UniversalRing{T, U}) where {T, U} = UniversalRingElem{T, U}(zero(base_ring(R)), R)

		one(R::UniversalRing{T, U}) where {T, U} = UniversalRingElem{T, U}(one(base_ring(R)), R)

Conversation

SoongNoonien commented Jan 7, 2026

Uh oh!

Uh oh!

SoongNoonien commented Jan 7, 2026

Uh oh!

SoongNoonien commented Jan 8, 2026

Uh oh!

lgoettgens commented Jan 8, 2026

Uh oh!

Uh oh!

SoongNoonien commented Jan 8, 2026

Uh oh!

lgoettgens commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fingolfin commented Jan 8, 2026

Uh oh!

fingolfin commented Jan 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SoongNoonien commented Jan 13, 2026

Uh oh!

This comment was marked as resolved.

codecov Bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgoettgens commented Mar 18, 2026

Uh oh!

SoongNoonien commented Mar 18, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lgoettgens commented Jan 8, 2026 •

edited

Loading

codecov Bot commented Jan 15, 2026 •

edited

Loading