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)
Change History (19)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
- 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 ofQQ[K.variable_name()]
. - The construction of
QQ[K.variable_name()]
also involves the registration of a coerce map fromQQ
toQQ[K.variable_name()]
. - Now, one constructs
K['x','y']
. Again,_singular_ring_new
is called. Again, it creates a new version ofQQ[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: ↓ 4 Changed 8 years ago by
Why would PolynomialRing
throw a TypeError
and MPolynomialRing_libsingular
would accept these parameters?
comment:4 in reply to: ↑ 3 Changed 8 years ago by
Replying to malb:
Why would
PolynomialRing
throw aTypeError
andMPolynomialRing_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
Ah, got it. Thanks, that makes sense. The patch reads good, I haven't applied & tested it though.
comment:6 follow-up: ↓ 8 Changed 8 years ago by
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
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
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
Also, small typo at the end of this line:
# the integers using the (cached) polynomial ring constructor:@@@
:)
comment:10 Changed 8 years ago by
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
The patchbot does not find actual errors (even though the blob is yellow). Review, anyone?
comment:12 Changed 8 years ago by
Martin, do you think you find the time to complete your review?
comment:13 Changed 8 years ago by
- Dependencies set to #11339
I'll have to rebase, because #11339 (which is merged) creates a conflict.
comment:14 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to rebase
Changed 8 years ago by
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
- Status changed from needs_work to needs_review
- Work issues rebase deleted
Done!
comment:16 Changed 8 years ago by
- 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
- Milestone changed from sage-4.8 to sage-5.0
comment:18 Changed 8 years ago by
- Merged in set to sage-5.0.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
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 beMPolynomialRing_libsingular
.