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: sage-6.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:

Status badges

Description (last modified by Darij Grinberg)

sage: R = LaurentPolynomialRing(ZZ, 'x')
sage: T = LaurentPolynomialRing(QQ, 'x')
sage: R.gen() + T.gen()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-dac3b0b430b6> in <module>()
----> 1 R.gen() + T.gen()

/home/darij/sage-5.13.beta0/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.RingElement.__add__ (sage/structure/element.c:13884)()

/home/darij/sage-5.13.beta0/local/lib/python2.7/site-packages/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 Darij Grinberg

Description: modified (diff)

comment:2 Changed 9 years ago by Travis Scrimshaw

Cc: Travis Scrimshaw added

comment:3 Changed 9 years ago by Frédéric Chapoton

I have just been hit by a similar problem:

sage: R = LaurentPolynomialRing(ZZ, 'x')
sage: T = PolynomialRing(ZZ, 'x')
sage: R.gen() + T.gen()
2*x
sage: R.gen() + FractionField(T).gen()
BOOM !

comment:4 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:5 Changed 9 years ago by Frédéric Chapoton

Branch: u/chapoton/15345
Commit: d35d7b06cc5389763aeca95d7e1044deea001eff

New commits:

d35d7b0trac #15345 first step: coercion of Laurent polynomials into fractions

comment:6 Changed 9 years ago by git

Commit: d35d7b06cc5389763aeca95d7e1044deea001eff3c151ec215be2f31df26c0089d967de585ea457b

Branch pushed to git repo; I updated commit sha1. New commits:

3c151ectrac #15345 corrected code

comment:7 Changed 9 years ago by git

Commit: 3c151ec215be2f31df26c0089d967de585ea457b62bf2e942c670fae6cc705126cced936e8cd268e

Branch pushed to git repo; I updated commit sha1. New commits:

62bf2e9trac #15345 better coercion

comment:8 Changed 9 years ago by Frédéric Chapoton

Status: newneeds_info

Here is something that seems to work. There remains to add doctests for the new behaviour.

comment:9 Changed 9 years ago by Frédéric Chapoton

Status: needs_infoneeds_work
Work issues: add doctests

comment:10 Changed 9 years ago by git

Commit: 62bf2e942c670fae6cc705126cced936e8cd268e6d4916c33c0dc3c96f34740ce511b750333fa781

Branch pushed to git repo; I updated commit sha1. New commits:

6d4916ctrac #15345 documentation added

comment:11 Changed 9 years ago by Frédéric Chapoton

Status: needs_workneeds_review

ok, should be good. Comments, anybody ?

comment:12 Changed 9 years ago by Frédéric Chapoton

Authors: Frédéric Chapoton

comment:13 Changed 9 years ago by Travis Scrimshaw

Status: needs_reviewneeds_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 Frédéric Chapoton

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 Changed 9 years ago by Frédéric 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 ?

comment:16 in reply to:  15 ; Changed 9 years ago by Travis Scrimshaw

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 sage-devel if everyone else agrees with doing this as things like numerator won't return Laurent polynomials.

comment:17 Changed 9 years ago by David Roe

Branch: u/chapoton/15345u/roed/ticket/15345
Created: Nov 1, 2013, 12:12:54 AMNov 1, 2013, 12:12:54 AM
Modified: Feb 12, 2014, 3:08:13 PMFeb 12, 2014, 3:08:13 PM

comment:18 in reply to:  16 Changed 9 years ago by David Roe

Authors: Frédéric ChapotonFrédéric Chapoton, David Roe
Commit: 6d4916c33c0dc3c96f34740ce511b750333fa7819cf030d93c04738161fdae4aec6914371a63f4b2
Status: needs_workneeds_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 sage-devel if everyone else agrees with doing this as things like numerator won't return Laurent polynomials.

I like it better too. Asking on sage-devel 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:

9cf030dUse fraction field of polynomial ring for fraction field of Laurent polynomial ring

comment:19 Changed 9 years ago by Frédéric Chapoton

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 Darij Grinberg

Both doctest failures in the Hall algebra file are exactly what should be happening. These doctests were illustrating a bug!

comment:21 Changed 9 years ago by Frédéric Chapoton

Branch: u/roed/ticket/15345u/chapoton/15345
Commit: 9cf030d93c04738161fdae4aec6914371a63f4b200393f332216d6d2b009842b02e1f9384ad0d807

I have changed the doctests in hall_algebra.

One should add tests for the new bahvior of FractionField.


New commits:

00393f3trac #15345 corrected doctests in hall algebra

comment:22 in reply to:  21 Changed 9 years ago by David Roe

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 Darij Grinberg

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 Frédéric Chapoton

@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 Travis Scrimshaw

Branch: u/chapoton/15345u/tscrim/ticket/15345
Commit: 00393f332216d6d2b009842b02e1f9384ad0d8078d541055054316406c45eb4ad44a068670bfe916
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 sage-devel, it's a positive review from me.


New commits:

f2e9d65Fixed documentation for hall_algebra.py.
8d54105Renamed to_fraction to _fraction_pair.

comment:26 Changed 9 years ago by David Roe

Status: needs_reviewpositive_review

Looks good to me.

comment:27 Changed 9 years ago by Volker Braun

Branch: u/tscrim/ticket/153458d541055054316406c45eb4ad44a068670bfe916
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.