polynomial quotient rings are unique parents

[saraedum]

Currently quotient rings are not unique parents which sometimes causes trouble with

sage: R.<x> = QQ[]
sage: R.quo(x) is R.quo(x)
False
sage: R.quo(x) == R.quo(x)
True

The changes proposed here make the creation go through a factory which makes sure that the parents are unique.

Depends on #23851

Component: commutative algebra

Keywords: sd86.5, sd87

Author: Julian Rüth

Branch/Commit: 966c36b

Reviewer: David Roe

Issue created by migration from https://trac.sagemath.org/ticket/22983

[saraedum]

Branch: u/saraedum/polynomial_quotient_rings_are_unique_parents

[nbruin]

Commit: 054945d

[nbruin]

comment:3

Does this fully solve the problem, though? One could give different generators for the same ideal. Should we be getting identical results for those, e.g., R.quo(x-1) and R.quo(2*x-2) (provided 2 is invertible in R ...). It might be worth documenting the choice made.

I suspect that for higher rank polynomial rings, normalizing the ideal representation might be too expensive (although computing in those rings would be expensive anyway).

---

New commits:

054945d

Make polynomial quotient rings unique parents

[saraedum]

comment:4

nbruin: You are right. It should be documented. I'll fix that.
I believe that your example should produce two different rings (at least for the time being.) These could be == equal but not is equal. However, getting == to work for them would require a change of the hash function which is (hard and) not the scope of this ticket.

[xcaruso]

comment:5

Currently R.quo(x-1) and R.quo(2*x-2) prints in the same way.

sage: R.<x>= QQ[]
sage: R.quo(x+1)
Univariate Quotient Polynomial Ring in xbar over Rational Field with modulus x + 1
sage: R.quo(2*x+2)
Univariate Quotient Polynomial Ring in xbar over Rational Field with modulus x + 1

So I would say rather that the two constructions should return the same ring; it should be easy, it suffices to make the defining polynomial monic because creating the key.

[saraedum]

comment:6

Replying to @xcaruso:

Currently R.quo(x-1) and R.quo(2*x-2) prints in the same way.

sage: R.<x>= QQ[]
sage: R.quo(x+1)
Univariate Quotient Polynomial Ring in xbar over Rational Field with modulus x + 1
sage: R.quo(2*x+2)
Univariate Quotient Polynomial Ring in xbar over Rational Field with modulus x + 1

So I would say rather that the two constructions should return the same ring; it should be easy, it suffices to make the defining polynomial monic because creating the key.

I agree. However, this is already happening automagically: quo creates a (principal) ideal and takes its generator as the modulus which is the same x+1 in both of your examples.
At the same time, if someone insists to create a quotient with the modulus x + 1 and one with the modulus 2*x + 2 by calling PolynomialQuotientRing directly, then these should be distinct because .modulus() is going to have a different value for the two, i.e., they are distinguishable. I'll add a doctest to clarify this.

[sagetrac-git]

Changed commit from 054945d to f6a1cd8

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f6a1cd8

Document normalization of modulus

[saraedum]

comment:9

Doctests in rings/polynomial pass for me. Let's see what the patchbot thinks about everything else.

[xcaruso]

comment:10

Ok.
It's a bit weird and confusing that the function PolynomialQuotientRing does not have the same behaviour than the method quo (and even QuotientRing), isn't it?

    sage: R.<x> = QQ[]
    sage: QuotientRing(R, 2*x+2)
    Univariate Quotient Polynomial Ring in xbar over Rational Field with modulus x + 1
    sage: PolynomialQuotientRing(R, 2*x+2)
    Univariate Quotient Polynomial Ring in xbar over Rational Field with modulus 2*x + 2

I would be in favour of uniformizing this... but it's definitely for another ticket.

I'll try to review your ticket soon.

[xcaruso]

comment:11

Why did you remove "in one variable" in the doctest.

    sage: R.<x,y> = QQ[]
    sage: PolynomialQuotientRing(R, x^2+y+1)
    ...
    TypeError: ring must be a polynomial ring

[saraedum]

comment:12

Replying to @xcaruso:

Why did you remove "in one variable" in the doctest.

    sage: R.<x,y> = QQ[]
    sage: PolynomialQuotientRing(R, x^2+y+1)
    ...
    TypeError: ring must be a polynomial ring

Because it already says "univariate" there.

[saraedum]

Changed keywords from none to sd86.5

[roed314]

comment:14

Tests all pass for me, and it looks good.

[roed314]

Reviewer: David Roe

[vbraun]

comment:15

Merge conflict

[roed314]

comment:16

Replying to @vbraun:

Merge conflict

For me, trac is showing it as still merging cleanly. Is there a new beta against which the merge is failing, or a ticket?

[saraedum]

comment:17

We do not get a merge conflict for that one.

[sagetrac-git]

Changed commit from 806d5af to 820b0a2

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f1f2fa5	Merge remote-tracking branch 'trac/u/saraedum/polynomial_quotient_rings_are_unique_parents' into trac-22983
820b0a2	Fix doctests

[saraedum]

comment:29

I thought for a while that the behaviour of these doctests was random. Actually
it is not. It is consistent over all patchbot runs with the same base version
of Sage. Caching the polynomial rings flips some signs, I am not sure why.
Anyway, the result is still correct and it is not random, so there is no need
for a _test_S_units() which would be hard to write anyway.

[sagetrac-git]

Changed commit from 820b0a2 to 4660c18

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4660c18

Fix doctests

[roed314]

comment:31

Looks good to me.

[vbraun]

comment:33

I still get failing tests. Random failures during high load. It looks like the tests depend on whether a garbage collection is forced due to low memory.

File "src/sage/rings/polynomial/polynomial_quotient_ring.py", line 1445, in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic.S_units
Failed example:
    L.S_units([])
Expected:
    [(1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
Got:
    [(-1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
**********************************************************************
File "src/sage/rings/polynomial/polynomial_quotient_ring.py", line 1449, in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic.S_units
Failed example:
    L.S_units([K.ideal(1/2*a - 3/2)])
Expected:
    [((-1/6*a - 1/2)*b^2 + (1/3*a - 1)*b + 4/3*a, +Infinity),
     (1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
Got:
    [((-1/6*a - 1/2)*b^2 + (1/3*a - 1)*b + 4/3*a, +Infinity),
     (-1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
**********************************************************************
File "src/sage/rings/polynomial/polynomial_quotient_ring.py", line 1454, in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic.S_units
Failed example:
    L.S_units([K.ideal(2)])
Expected:
    [((1/2*a - 1/2)*b^2 + (a + 1)*b + 3, +Infinity),
     ((1/6*a + 1/2)*b^2 + (-1/3*a + 1)*b - 5/6*a + 1/2, +Infinity),
     ((1/6*a + 1/2)*b^2 + (-1/3*a + 1)*b - 5/6*a - 1/2, +Infinity),
     (1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
Got:
    [((1/2*a - 1/2)*b^2 + (a + 1)*b + 3, +Infinity),
     ((1/6*a + 1/2)*b^2 + (-1/3*a + 1)*b - 5/6*a + 1/2, +Infinity),
     ((1/6*a + 1/2)*b^2 + (-1/3*a + 1)*b - 5/6*a - 1/2, +Infinity),
     (-1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
**********************************************************************
File "src/sage/rings/polynomial/polynomial_quotient_ring.py", line 1535, in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic.units
Failed example:
    L.units()
Expected:
    [(1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
Got:
    [(-1/2*a + 1/2, 6),
     ((-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, +Infinity),
     (2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2, +Infinity)]
**********************************************************************
File "src/sage/rings/polynomial/polynomial_quotient_ring.py", line 1544, in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic.units
Failed example:
    L.unit_group().gens_values()
Expected:
    [-1/2*a + 1/2, (-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, 2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2]
Got:
    [1/2*a + 1/2, (-1/3*a - 1)*b^2 - 4/3*a*b - 5/6*a + 7/2, 2/3*a*b^2 + (2/3*a - 2)*b - 5/6*a - 7/2]
**********************************************************************
2 items had failures:
   3 of  18 in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic.S_units
   2 of  23 in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_generic.units
    [451 tests, 5 failures, 2.92 s]
----------------------------------------------------------------------
sage -t --long src/sage/rings/polynomial/polynomial_quotient_ring.py  # 5 doctests failed
----------------------------------------------------------------------

[saraedum]

Changed branch from u/saraedum/polynomial_quotient_rings_are_unique_parents to none

[saraedum]

Changed commit from 4660c18 to none

[saraedum]

Branch: u/saraedum/22983

[sagetrac-git]

Commit: 966c36b

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

966c36b

Merge develop and 22983

[saraedum]

comment:37

Not the first time, something like this has happened: #23851 comment:5

---

New commits:

054945d	Make polynomial quotient rings unique parents
f6a1cd8	Document normalization of modulus
19af059	Merge branch 'develop' into t/22983/polynomial_quotient_rings_are_unique_parents
b04e7e9	Merge branch 'develop' into t/22983/polynomial_quotient_rings_are_unique_parents
272e7e3	Merge remote-tracking branch 'origin/develop' into HEAD
806d5af	Pickling now works
f1f2fa5	Merge remote-tracking branch 'trac/u/saraedum/polynomial_quotient_rings_are_unique_parents' into trac-22983
4660c18	Fix doctests
697d02e	Merge remote-tracking branch 'trac/u/saraedum/polynomial_quotient_rings_are_unique_parents' into HEAD
966c36b	Merge develop and 22983

[saraedum]

Dependencies: #23851

[saraedum]

comment:38

The issue has been fixed in #23851. Since I did not change anything, I am setting this back to positive review.

[vbraun]

Changed branch from u/saraedum/22983 to 966c36b