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

Priority:  major  Milestone:  sage6.2 
Component:  algebra  Keywords:  laurent polynomials, polynomials, coercion, categories 
Cc:  Sage Combinat CC user, Mike Hansen, William Stein, David Roe, Travis Scrimshaw  Merged in:  
Authors:  Frédéric Chapoton, David Roe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  add doctests 
Branch:  8d54105 (Commits, GitHub, GitLab)  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 9 years ago by
Description:  modified (diff) 

comment:2 Changed 9 years ago by
Cc:  Travis Scrimshaw added 

comment:3 Changed 9 years ago by
comment:4 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:5 Changed 9 years ago by
Branch:  → u/chapoton/15345 

Commit:  → d35d7b06cc5389763aeca95d7e1044deea001eff 
New commits:
d35d7b0  trac #15345 first step: coercion of Laurent polynomials into fractions

comment:6 Changed 9 years ago by
Commit:  d35d7b06cc5389763aeca95d7e1044deea001eff → 3c151ec215be2f31df26c0089d967de585ea457b 

Branch pushed to git repo; I updated commit sha1. New commits:
3c151ec  trac #15345 corrected code

comment:7 Changed 9 years ago by
Commit:  3c151ec215be2f31df26c0089d967de585ea457b → 62bf2e942c670fae6cc705126cced936e8cd268e 

Branch pushed to git repo; I updated commit sha1. New commits:
62bf2e9  trac #15345 better coercion

comment:8 Changed 9 years ago by
Status:  new → needs_info 

Here is something that seems to work. There remains to add doctests for the new behaviour.
comment:9 Changed 9 years ago by
Status:  needs_info → needs_work 

Work issues:  → add doctests 
comment:10 Changed 9 years ago by
Commit:  62bf2e942c670fae6cc705126cced936e8cd268e → 6d4916c33c0dc3c96f34740ce511b750333fa781 

Branch pushed to git repo; I updated commit sha1. New commits:
6d4916c  trac #15345 documentation added

comment:11 Changed 9 years ago by
Status:  needs_work → needs_review 

ok, should be good. Comments, anybody ?
comment:12 Changed 9 years ago by
Authors:  → Frédéric Chapoton 

comment:13 Changed 9 years ago by
Status:  needs_review → 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 9 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 9 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 followup: 18 Changed 9 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 9 years ago by
Branch:  u/chapoton/15345 → u/roed/ticket/15345 

Created:  Nov 1, 2013, 12:12:54 AM → Nov 1, 2013, 12:12:54 AM 
Modified:  Feb 12, 2014, 3:08:13 PM → Feb 12, 2014, 3:08:13 PM 
comment:18 Changed 9 years ago by
Authors:  Frédéric Chapoton → Frédéric Chapoton, David Roe 

Commit:  6d4916c33c0dc3c96f34740ce511b750333fa781 → 9cf030d93c04738161fdae4aec6914371a63f4b2 
Status:  needs_work → 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 9 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 9 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 9 years ago by
Branch:  u/roed/ticket/15345 → u/chapoton/15345 

Commit:  9cf030d93c04738161fdae4aec6914371a63f4b2 → 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 Changed 9 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 9 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 9 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 9 years ago by
Branch:  u/chapoton/15345 → u/tscrim/ticket/15345 

Commit:  00393f332216d6d2b009842b02e1f9384ad0d807 → 8d541055054316406c45eb4ad44a068670bfe916 
Reviewers:  → 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:27 Changed 9 years ago by
Branch:  u/tscrim/ticket/15345 → 8d541055054316406c45eb4ad44a068670bfe916 

Resolution:  → fixed 
Status:  positive_review → closed 
I have just been hit by a similar problem: