Creating a polynomial ring over a number field results in a non-unique polynomial ring over the rationals

[SimonKing]

The problem is the following code in sage.libs.singular.ring.singular_ring_new:

    elif PY_TYPE_CHECK(base_ring, FiniteField_generic):
        if base_ring.characteristic() <= 2147483629:
            characteristic = -base_ring.characteristic() # note the negative characteristic
        else:
            raise TypeError, "characteristic must be <= 2147483629."
        # TODO: This is lazy, it should only call Singular stuff not MPolynomial stuff
        k = MPolynomialRing_libsingular(base_ring.prime_subfield(), 1, base_ring.variable_name(), 'lex')
        minpoly = base_ring.polynomial()(k.gen())
        is_extension = True

    elif PY_TYPE_CHECK(base_ring, NumberField) and base_ring.is_absolute():
        characteristic = 1
        k = MPolynomialRing_libsingular(RationalField(), 1, base_ring.variable_name(), 'lex')
        minpoly = base_ring.polynomial()(k.gen())
        is_extension = True

Hence, a multivariate libsingular polynomial ring is constructed without using the (cached) polynomial ring constructor. The comment should rather not be "This is lazy,..." but "This is dangerous, it should only call polynomial ring constructor stuff, not MPolynomial stuff".

I am trying to find an example in unpatched Sage where this is actually a problem. However, while working at #10667, the non-unique parents led to several hundred doctest errors in elliptic curves.

[SimonKing]

I suggest to call the polynomial ring constructor (which might actually be a little faster, since it is cached) and to catch the TypeError that would occur if the resulting ring happens to not be MPolynomialRing_libsingular.

[SimonKing]

I did not run tests yet, and I am not able to provide a doc test that shows that the bug is fixed. But it turns out that the patch that I just attached solves the problem I met while working at #10667, and thus I change it into "needs review".

The problem was as follows. Let K.<i> = NumberField(x^2+1).

• During initialisation of K['x','y','z'], by one of the patches in my patch chain, there is also an initialisation of a coercion from the base ring.
• The construction of the coercion results in a call to _singular_ring_new. It constructs a non-unique libsingular version of QQ[K.variable_name()].
• The construction of QQ[K.variable_name()] also involves the registration of a coerce map from QQ to QQ[K.variable_name()].
• Now, one constructs K['x','y']. Again, _singular_ring_new is called. Again, it creates a new version of QQ[K.variable_name()], and again it tries to register the coercion. But the coercion is already contained in the global coercion cache. Error.

[malb]

Why would PolynomialRing throw a TypeError and MPolynomialRing_libsingular would accept these parameters?

[SimonKing]

Replying to malb:

Why would PolynomialRing throw a TypeError and MPolynomialRing_libsingular would accept these parameters?

The TypeError would not be raised by PolynomialRing.

I was thinking about future developments. Imaging that someone plays with the polynomial ring constructor, such that a completely different class of polynomial rings would be returned. In that case, the attempt to assign the ring to the cdef variable k of type MPolynomialRing_libsingular would result in a TypeError.

[malb]

Ah, got it. Thanks, that makes sense. The patch reads good, I haven't applied & tested it though.

[malb]

Doctests pass. However, I gave the TypeError thing another thought. If somebody changes the class for polynomials over the base field (prime field or rational field), couldn't that again lead to problems with non-identical parents? I.e. should we cause the TypeError such that future developers will note there's a potential problem?

[SimonKing]

I fixed another bug caused by a rogue creation of a polynomial ring: It was in the numerator() method of libsingular polynomials.

The patch, that I updated a few seconds ago, contains a new doctest:

        sage: P.<foo,bar> = ZZ[]
        sage: Q.<foo,bar> = QQ[]
        sage: f = Q.random_element()
        sage: f.numerator().parent() is P
        True

The last answer was False in unpatched sage.

[SimonKing]

Replying to malb:

Doctests pass. However, I gave the TypeError thing another thought. If somebody changes the class for polynomials over the base field (prime field or rational field), couldn't that again lead to problems with non-identical parents? I.e. should we cause the TypeError such that future developers will note there's a potential problem?

Sorry, I saw your answer only now.

Yes, on second thought, it seems reasonable to not catch the type error.

I am sorry that our messages crossed (I have updated my patch, it now contains a second fix and one doctest). I will remove catching the type error in a minute.

[malb]

Also, small typo at the end of this line:

 # the integers using the (cached) polynomial ring constructor:@@@ 

:)

[SimonKing]

I removed the typo (actually it is not a typo but a mark, in order to more easily find the spot that I was working on).

I am still catching the type error, but am re-raising it with an error message that is hopefully clear enough. Well, hopefully nobody will ever see it...

[SimonKing]

The patchbot does not find actual errors (even though the blob is yellow). Review, anyone?

[SimonKing]

Martin, do you think you find the time to complete your review?

[SimonKing]

I'll have to rebase, because #11339 (which is merged) creates a conflict.

[SimonKing]

Avoid the creation of a non-unique auxiliary polynomial ring in singular_ring_new and in MPolynomial_libsingular.numerator

[SimonKing]

Done!

[davidloeffler]

This looks fine to me, and all tests pass under 4.8.alpha5.