Opened 8 years ago

Closed 8 years ago

#11780 closed defect (fixed)

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

Reported by: SimonKing Owned by: robertwb
Priority: major Milestone: sage-5.0
Component: coercion Keywords: non-unique polynomial ring number field
Cc: malb Merged in: sage-5.0.beta1
Authors: Simon King Reviewers: Martin Albrecht, David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11339 Stopgaps:

Description

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.

Attachments (1)

trac11780_unique_auxiliar_polyring.patch (3.8 KB) - added by SimonKing 8 years ago.
Avoid the creation of a non-unique auxiliary polynomial ring in singular_ring_new and in MPolynomial_libsingular.numerator

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by SimonKing

  • Authors set to Simon King
  • Status changed from new to needs_review

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.

comment:3 follow-up: Changed 8 years ago by malb

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

comment:4 in reply to: ↑ 3 Changed 8 years ago by 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.

comment:5 Changed 8 years ago by malb

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

comment:6 follow-up: Changed 8 years ago by 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?

comment:7 Changed 8 years ago by 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.

comment:8 in reply to: ↑ 6 Changed 8 years ago by 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.

comment:9 Changed 8 years ago by malb

Also, small typo at the end of this line:

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

:)

comment:10 Changed 8 years ago by 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...

comment:11 Changed 8 years ago by SimonKing

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

comment:12 Changed 8 years ago by SimonKing

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

comment:13 Changed 8 years ago by SimonKing

  • Dependencies set to #11339

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

comment:14 Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to rebase

Changed 8 years ago by SimonKing

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

comment:15 Changed 8 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues rebase deleted

Done!

comment:16 Changed 8 years ago by davidloeffler

  • Reviewers set to Martin Albrecht, David Loeffler
  • Status changed from needs_review to positive_review

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

comment:17 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

comment:18 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.