Opened 9 years ago

Closed 9 years ago

# Laurent polynomial rings don't preserve coercion

Reported by: Owned by: Darij Grinberg major sage-6.2 algebra laurent polynomials, polynomials, coercion, categories Sage Combinat CC user, Mike Hansen, William Stein, David Roe, Travis Scrimshaw Frédéric Chapoton, David Roe Travis Scrimshaw N/A add doctests 8d54105 8d541055054316406c45eb4ad44a068670bfe916

```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/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
```

### comment:1 Changed 9 years ago by Darij Grinberg

Description: modified (diff)

### 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.1 → sage-6.2

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

Branch: → u/chapoton/15345 → d35d7b06cc5389763aeca95d7e1044deea001eff

New commits:

 ​d35d7b0 `trac #15345 first step: coercion of Laurent polynomials into fractions`

### comment:6 Changed 9 years ago by git

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 git

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

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

Status: needs_info → needs_work → add doctests

### comment:10 Changed 9 years ago by git

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

Status: needs_work → needs_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_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 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 follow-up:  16 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 ; follow-up:  18 Changed 9 years ago by Travis Scrimshaw

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/15345 → u/roed/ticket/15345 Nov 1, 2013, 12:12:54 AM → Nov 1, 2013, 12:12:54 AM Feb 12, 2014, 3:08:13 PM → Feb 12, 2014, 3:08:13 PM

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

Authors: Frédéric Chapoton → Frédéric Chapoton, David Roe 6d4916c33c0dc3c96f34740ce511b750333fa781 → 9cf030d93c04738161fdae4aec6914371a63f4b2 needs_work → needs_review

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:

 ​9cf030d `Use 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 follow-up:  22 Changed 9 years ago by Frédéric Chapoton

Branch: u/roed/ticket/15345 → u/chapoton/15345 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 in reply to:  21 Changed 9 years ago by David Roe

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

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:

 ​f2e9d65 `Fixed documentation for hall_algebra.py.` ​8d54105 `Renamed to_fraction to _fraction_pair.`

### comment:26 Changed 9 years ago by David Roe

Status: needs_review → positive_review

Looks good to me.

### comment:27 Changed 9 years ago by Volker Braun

Branch: u/tscrim/ticket/15345 → 8d541055054316406c45eb4ad44a068670bfe916 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.