Opened 6 years ago
Closed 6 years ago
#15345 closed defect (fixed)
Laurent polynomial rings don't preserve coercion
Reported by:  darij  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  algebra  Keywords:  laurent polynomials, polynomials, coercion, categories 
Cc:  sagecombinat, mhansen, was, roed, tscrim  Merged in:  
Authors:  Frédéric Chapoton, David Roe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  add doctests 
Branch:  8d54105 (Commits)  Commit:  8d541055054316406c45eb4ad44a068670bfe916 
Dependencies:  Stopgaps: 
Description (last modified by )
sage: R = LaurentPolynomialRing(ZZ, 'x') sage: T = LaurentPolynomialRing(QQ, 'x') sage: R.gen() + T.gen()  TypeError Traceback (most recent call last) <ipythoninput3dac3b0b430b6> in <module>() > 1 R.gen() + T.gen() /home/darij/sage5.13.beta0/local/lib/python2.7/sitepackages/sage/structure/element.so in sage.structure.element.RingElement.__add__ (sage/structure/element.c:13884)() /home/darij/sage5.13.beta0/local/lib/python2.7/sitepackages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:8169)() TypeError: unsupported operand parent(s) for '+': 'Univariate Laurent Polynomial Ring in x over Integer Ring' and 'Univariate Laurent Polynomial Ring in x over Rational Field'
Compare with the more complicated:
sage: R = PolynomialRing(ZZ, 'x').fraction_field() sage: T = PolynomialRing(QQ, 'x').fraction_field() sage: R.gen() + T.gen() 2*x
Change History (27)
comment:1 Changed 6 years ago by
 Description modified (diff)
comment:2 Changed 6 years ago by
 Cc tscrim added
comment:3 Changed 6 years ago by
comment:4 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:5 Changed 6 years ago by
 Branch set to u/chapoton/15345
 Commit set to d35d7b06cc5389763aeca95d7e1044deea001eff
New commits:
d35d7b0  trac #15345 first step: coercion of Laurent polynomials into fractions

comment:6 Changed 6 years ago by
 Commit changed from d35d7b06cc5389763aeca95d7e1044deea001eff to 3c151ec215be2f31df26c0089d967de585ea457b
Branch pushed to git repo; I updated commit sha1. New commits:
3c151ec  trac #15345 corrected code

comment:7 Changed 6 years ago by
 Commit changed from 3c151ec215be2f31df26c0089d967de585ea457b to 62bf2e942c670fae6cc705126cced936e8cd268e
Branch pushed to git repo; I updated commit sha1. New commits:
62bf2e9  trac #15345 better coercion

comment:8 Changed 6 years ago by
 Status changed from new to needs_info
Here is something that seems to work. There remains to add doctests for the new behaviour.
comment:9 Changed 6 years ago by
 Status changed from needs_info to needs_work
 Work issues set to add doctests
comment:10 Changed 6 years ago by
 Commit changed from 62bf2e942c670fae6cc705126cced936e8cd268e to 6d4916c33c0dc3c96f34740ce511b750333fa781
Branch pushed to git repo; I updated commit sha1. New commits:
6d4916c  trac #15345 documentation added

comment:11 Changed 6 years ago by
 Status changed from needs_work to needs_review
ok, should be good. Comments, anybody ?
comment:12 Changed 6 years ago by
comment:13 Changed 6 years ago by
 Status changed from needs_review to needs_work
This breaks division:
sage: T.<x> = LaurentPolynomialRing(QQ) sage: 1 / x  TypeError Traceback (most recent call last) ... TypeError: <lambda>() takes exactly 1 argument (2 given)
comment:14 Changed 6 years ago by
Too bad.. Maybe then one can content ourselves to solve the initial problem, and not try to solve the problem about the fraction field, which seems to be harder ?
comment:15 followup: ↓ 16 Changed 6 years ago by
Maybe one could also ensure that the fraction field of a Laurent Polynomial ring is just the fraction field of its associated polynomial ring ?
comment:16 in reply to: ↑ 15 ; followup: ↓ 18 Changed 6 years ago by
Replying to chapoton:
Maybe one could also ensure that the fraction field of a Laurent Polynomial ring is just the fraction field of its associated polynomial ring ?
I like this idea as the usual polynomial rings are much better behaved as a fraction field compared to their Laurent counterparts (even with #11726). However I think we should ask on sagedevel if everyone else agrees with doing this as things like numerator
won't return Laurent polynomials.
comment:17 Changed 6 years ago by
 Branch changed from u/chapoton/15345 to u/roed/ticket/15345
 Created changed from 11/01/13 00:12:54 to 11/01/13 00:12:54
 Modified changed from 02/12/14 15:08:13 to 02/12/14 15:08:13
comment:18 in reply to: ↑ 16 Changed 6 years ago by
 Commit changed from 6d4916c33c0dc3c96f34740ce511b750333fa781 to 9cf030d93c04738161fdae4aec6914371a63f4b2
 Status changed from needs_work to needs_review
Replying to tscrim:
Replying to chapoton:
Maybe one could also ensure that the fraction field of a Laurent Polynomial ring is just the fraction field of its associated polynomial ring ?
I like this idea as the usual polynomial rings are much better behaved as a fraction field compared to their Laurent counterparts (even with #11726). However I think we should ask on sagedevel if everyone else agrees with doing this as things like
numerator
won't return Laurent polynomials.
I like it better too. Asking on sagedevel might be a good plan though. I've made a branch that does this, and fixes the bug with 1/x
observed above.
New commits:
9cf030d  Use fraction field of polynomial ring for fraction field of Laurent polynomial ring

comment:19 Changed 6 years ago by
Travis, what do you think of the two failing doctests in HallAlgebra? (see patchbot report) ?
The first one looks innocent enough, but the second one seems rather strange to me.
comment:20 Changed 6 years ago by
Both doctest failures in the Hall algebra file are exactly what should be happening. These doctests were illustrating a bug!
comment:21 followup: ↓ 22 Changed 6 years ago by
 Branch changed from u/roed/ticket/15345 to u/chapoton/15345
 Commit changed from 9cf030d93c04738161fdae4aec6914371a63f4b2 to 00393f332216d6d2b009842b02e1f9384ad0d807
I have changed the doctests in hall_algebra.
One should add tests for the new bahvior of FractionField
.
New commits:
00393f3  trac #15345 corrected doctests in hall algebra

comment:22 in reply to: ↑ 21 Changed 6 years ago by
Replying to chapoton:
I have changed the doctests in hall_algebra.
One should add tests for the new bahvior of
FractionField
.
I added some tests in the fraction_field
method. Were you thinking of others?
comment:23 Changed 6 years ago by
Two comments, not a review (I fear I have no idea what spooky action at a distance these changes might cause):
 In
hall_algebra.py
, please change not just the doctests but the whole WARNING they're in. They should be a regular example, not a WARNING.
 Could you maybe document
converter
? I know it's an internal function, but I want to understand what it does and why.
comment:24 Changed 6 years ago by
@David: I was thinking to something like
sage: FractionField(LaurentPolynomialRing(QQ,'x')) Fraction Field of Multivariate Polynomial Ring in x over Rational Field
but, yes, this can be considered as superfluous.
@Darij, yes, I agree that one should change the doc in hall_algebra in a better way. I was hoping that somebody more knowledgeable than me about this part of sage would do that.
comment:25 Changed 6 years ago by
 Branch changed from u/chapoton/15345 to u/tscrim/ticket/15345
 Commit changed from 00393f332216d6d2b009842b02e1f9384ad0d807 to 8d541055054316406c45eb4ad44a068670bfe916
 Reviewers set to Travis Scrimshaw
Okay, I've fixed up the Hall algebra documentation. I've changed to_fraction()
into _fraction_pair()
since this is only suppose to be used for the coercion (so it's hidden) and IMO it more accurately reflects what is being returned. If you're happy with my changes, then up to someone not likely what we're doing on sagedevel, it's a positive review from me.
New commits:
f2e9d65  Fixed documentation for hall_algebra.py.

8d54105  Renamed to_fraction to _fraction_pair.

comment:26 Changed 6 years ago by
 Status changed from needs_review to positive_review
Looks good to me.
comment:27 Changed 6 years ago by
 Branch changed from u/tscrim/ticket/15345 to 8d541055054316406c45eb4ad44a068670bfe916
 Resolution set to fixed
 Status changed from positive_review to closed
I have just been hit by a similar problem: