Opened 4 years ago

Closed 4 years ago

# Conversion problem between Laurent polynomial ring and its field of fractions

Reported by: Owned by: Sebastian Oehms major sage-8.9 algebra coercion, laurent plynomial, field of fraction, days100 Travis Scrimshaw, Simon King Sebastian Oehms Simon King, Vincent Delecroix N/A 38ef901 38ef90172309bc99d0eade264de9bbe603da2404

The issue is explained by the following example:

```sage: R.<t> = LaurentPolynomialRing(ZZ)
sage: F = FractionField(R)
sage: R(F(1/t))
Traceback (most recent call last):
...
TypeError: fraction must have unit denominator
```

There is a natural conversion back to the Laurent polynomials. Most likely the denominator is not being checked if it is a unit in `R`.

### comment:1 Changed 4 years ago by Travis Scrimshaw

Description: modified (diff) Coercion problem between Laurent polynomial ring and its field of fraction → Conversion problem between Laurent polynomial ring and its field of fractions

This is not a coercion, but a conversion (as it does not have to be defined going back to `R`).

### comment:2 Changed 4 years ago by Sebastian Oehms

Description: modified (diff)

### comment:4 Changed 4 years ago by Vincent Delecroix

Milestone: sage-8.4 → sage-8.9

### comment:5 Changed 4 years ago by Sebastian Oehms

Branch: → u/soehms/conversion_problem_laurent_fraction_field_26425

### comment:6 Changed 4 years ago by Sebastian Oehms

Commit: → 37161517cfcc15caf415816cd4dfbd4eda73f0b2 new → needs_review

The check for being a unit is performed in the section map of the natural inclusion of a ring into its ring of fractions. Thus, since this ring is not the Laurent polynomial ring itself but the corresponding polynomial ring, that test fails.

My attempt for a fix is to implement the conversion in the `_element_constructor` of the Laurent polynomial ring!

New commits:

 ​3716151 `26425: initial`

### comment:8 follow-up:  10 Changed 4 years ago by Simon King

Note this:

```sage: R.<t> = LaurentPolynomialRing(ZZ)
sage: t.is_unit()
True
```

So, things SHOULD work, as the denominator of `1/t` is a unit. But:

```sage: T = F(1/t)
sage: T.denominator().parent()
Univariate Polynomial Ring in t over Integer Ring
sage: R.fraction_field() is R.polynomial_ring().fraction_field()
True
```

Isn't that a bug? EDIT: No, because mathematically the two fraction fields are isomorphic.

Actually I recall that a couple of years ago I tried to let `FractionField(R)` be the `LaurentSeriesRing`, which mathematically is correct. EDIT: ... correct in the same way as the current fraction field is correct.

I think the following patch is not optimal:

```+            P = x.parent()
+            if P.ring() == self.polynomial_ring():
+                d = self(x.denominator())
+                if not d.is_unit():
+                     raise TypeError("fraction must have unit denominator")
+                return self(x.numerator()) * d.inverse_of_unit()
```

I think we don't want to test that `P.ring() == self.polynomial_ring()`, but it is enough that `self.polynomial_ring().has_coerce_map_from(P.ring())`.

Or even better: We are talking about conversion here. So, there is no need to test whether the denominator coerces into `self`. We could just do

```+        elif isinstance(x, FractionFieldElement):
+            # since the field of fraction of self is defined corresponding to the polynomial ring of self
+            # the conversion of its elements back must be treated separately (:trac:`26425`).
+            d = self(x.denominator())
+            if not d.is_unit():
+                 raise TypeError("fraction must have unit denominator")
+            return self(x.numerator()) * d.inverse_of_unit()
```
Last edited 4 years ago by Simon King (previous) (diff)

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

Commit: 37161517cfcc15caf415816cd4dfbd4eda73f0b2 → 2058e65fdc44784dcd34bc6ed000d0815ef8352a

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

 ​2058e65 `26425: correction accordng to reviewers suggestion`

### comment:10 in reply to:  8 Changed 4 years ago by Sebastian Oehms

Or even better: We are talking about conversion here. So, there is no need to test whether the denominator coerces into `self`. We could just do

```+        elif isinstance(x, FractionFieldElement):
+            # since the field of fraction of self is defined corresponding to the polynomial ring of self
+            # the conversion of its elements back must be treated separately (:trac:`26425`).
+            d = self(x.denominator())
+            if not d.is_unit():
+                 raise TypeError("fraction must have unit denominator")
+            return self(x.numerator()) * d.inverse_of_unit()
```

As we talked before, the check was just matter of caution. I agree to have that removed!

### comment:11 Changed 4 years ago by Vincent Delecroix

You need two `:` at the end of this line

```+        Check that conversion back from fraction field does work (:trac:`26425`):
```

### comment:12 Changed 4 years ago by Simon King

Branch: u/soehms/conversion_problem_laurent_fraction_field_26425 → u/SimonKing/conversion_problem_laurent_fraction_field_26425

### comment:13 Changed 4 years ago by Simon King

Authors: → Sebastian Oehms 2058e65fdc44784dcd34bc6ed000d0815ef8352a → 38ef90172309bc99d0eade264de9bbe603da2404 → Simon King, Vincent Delecroix

I fixed the missing `::`.

All but one tests pass. The failing test is

```king@klap:~/Sage/git/sage\$ ./sage -t --warn-long 47.6 src/doc/common/conf.py  # 1 doctest failed

Running doctests with ID 2019-07-25-00-01-10-c2c7af32.
Git branch: t/26425/conversion_problem_laurent_fraction_field_26425
Using --optional=build,ccache,dochtml,frobby,gap_packages,gdb,gfortran,meataxe,memlimit,mpir,p_group_cohomology,python2,sage
Doctesting 1 file.
sage -t --warn-long 47.6 src/doc/common/conf.py
**********************************************************************
File "src/doc/common/conf.py", line 662, in doc.common.conf.call_intersphinx
Failed example:
for line in open(thematic_index).readlines():  # optional - dochtml
_ = sys.stdout.write(line)
Expected:
Got:
**********************************************************************
1 of   4 in doc.common.conf.call_intersphinx
[3 tests, 1 failure, 0.01 s]
----------------------------------------------------------------------
sage -t --warn-long 47.6 src/doc/common/conf.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.1 seconds
cpu time: 0.0 seconds
cumulative wall time: 0.0 seconds
```

Is that related? Probably not. But before I give a positive review, I'd like to be sure that the error has not been introduced by this ticket.

New commits:

 ​38ef901 `Fix a typo in the docs`

### comment:14 Changed 4 years ago by Simon King

Status: needs_review → positive_review

Indeed:

```king@klap:~/Sage/git/sage\$ git checkout 8.9.beta3
HEAD is now at a1e1a8f... Updated SageMath version to 8.9.beta3
king@klap:~/Sage/git/sage\$ ./sage -b
...
king@klap:~/Sage/git/sage\$ ./sage -t --warn-long 47.6 src/doc/common/conf.py
Running doctests with ID 2019-07-25-00-12-53-64443448.
Using --optional=build,ccache,dochtml,frobby,gap_packages,gdb,gfortran,meataxe,memlimit,mpir,p_group_cohomology,python2,sage
Doctesting 1 file.
sage -t --warn-long 47.6 src/doc/common/conf.py
**********************************************************************
File "src/doc/common/conf.py", line 662, in doc.common.conf.call_intersphinx
Failed example:
for line in open(thematic_index).readlines():  # optional - dochtml
_ = sys.stdout.write(line)
Expected:
Got:
**********************************************************************
1 of   4 in doc.common.conf.call_intersphinx
[3 tests, 1 failure, 0.01 s]
----------------------------------------------------------------------
sage -t --warn-long 47.6 src/doc/common/conf.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.1 seconds
cpu time: 0.0 seconds
cumulative wall time: 0.0 seconds
```

Since this error has not been introduced by this ticket, I think I can give a positive review. Unless Vincent or someone else disagrees.

### comment:15 Changed 4 years ago by Sebastian Oehms

Thanks a lot, Simon!

### comment:16 Changed 4 years ago by Volker Braun

Branch: u/SimonKing/conversion_problem_laurent_fraction_field_26425 → 38ef90172309bc99d0eade264de9bbe603da2404 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.