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: sage-6.2
Component: algebra Keywords: laurent polynomials, polynomials, coercion, categories
Cc: sage-combinat, 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 darij)

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 6 years ago by darij

  • Description modified (diff)

comment:2 Changed 6 years ago by tscrim

  • Cc tscrim added

comment:3 Changed 6 years ago by 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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:5 Changed 6 years ago by chapoton

  • Branch set to u/chapoton/15345
  • Commit set to d35d7b06cc5389763aeca95d7e1044deea001eff

New commits:

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

comment:6 Changed 6 years ago by git

  • Commit changed from d35d7b06cc5389763aeca95d7e1044deea001eff to 3c151ec215be2f31df26c0089d967de585ea457b

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

3c151ectrac #15345 corrected code

comment:7 Changed 6 years ago by git

  • Commit changed from 3c151ec215be2f31df26c0089d967de585ea457b to 62bf2e942c670fae6cc705126cced936e8cd268e

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

62bf2e9trac #15345 better coercion

comment:8 Changed 6 years ago by chapoton

  • 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 chapoton

  • Status changed from needs_info to needs_work
  • Work issues set to add doctests

comment:10 Changed 6 years ago by git

  • Commit changed from 62bf2e942c670fae6cc705126cced936e8cd268e to 6d4916c33c0dc3c96f34740ce511b750333fa781

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

6d4916ctrac #15345 documentation added

comment:11 Changed 6 years ago by chapoton

  • Status changed from needs_work to needs_review

ok, should be good. Comments, anybody ?

comment:12 Changed 6 years ago by chapoton

  • Authors set to Frédéric Chapoton

comment:13 Changed 6 years ago by tscrim

  • 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 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 follow-up: Changed 6 years ago by 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 ; follow-up: Changed 6 years ago by 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.

comment:17 Changed 6 years ago by roed

  • 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 roed

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, David Roe
  • 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 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 6 years ago by 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 6 years ago by darij

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

comment:21 follow-up: Changed 6 years ago by chapoton

  • 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:

00393f3trac #15345 corrected doctests in hall algebra

comment:22 in reply to: ↑ 21 Changed 6 years ago by roed

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 darij

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 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 6 years ago by tscrim

  • 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 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 6 years ago by roed

  • Status changed from needs_review to positive_review

Looks good to me.

comment:27 Changed 6 years ago by vbraun

  • Branch changed from u/tscrim/ticket/15345 to 8d541055054316406c45eb4ad44a068670bfe916
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.