Opened 2 years ago

Closed 18 months ago

#26425 closed defect (fixed)

Conversion problem between Laurent polynomial ring and its field of fractions

Reported by: soehms Owned by:
Priority: major Milestone: sage-8.9
Component: algebra Keywords: coercion, laurent plynomial, field of fraction, days100
Cc: tscrim, SimonKing Merged in:
Authors: Sebastian Oehms Reviewers: Simon King, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 38ef901 (Commits) Commit: 38ef90172309bc99d0eade264de9bbe603da2404
Dependencies: Stopgaps:

Description (last modified by soehms)

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.

Change History (16)

comment:1 Changed 2 years ago by tscrim

  • Description modified (diff)
  • Summary changed from Coercion problem between Laurent polynomial ring and its field of fraction to 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 2 years ago by soehms

  • Description modified (diff)

comment:3 Changed 18 months ago by soehms

  • Keywords days100 added

comment:4 Changed 18 months ago by vdelecroix

  • Milestone changed from sage-8.4 to sage-8.9

comment:5 Changed 18 months ago by soehms

  • Branch set to u/soehms/conversion_problem_laurent_fraction_field_26425

comment:6 Changed 18 months ago by soehms

  • Commit set to 37161517cfcc15caf415816cd4dfbd4eda73f0b2
  • Status changed from new to 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:

371615126425: initial

comment:7 Changed 18 months ago by SimonKing

  • Cc SimonKing added

comment:8 follow-up: Changed 18 months ago by SimonKing

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 18 months ago by SimonKing (previous) (diff)

comment:9 Changed 18 months ago by git

  • Commit changed from 37161517cfcc15caf415816cd4dfbd4eda73f0b2 to 2058e65fdc44784dcd34bc6ed000d0815ef8352a

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

2058e6526425: correction accordng to reviewers suggestion

comment:10 in reply to: ↑ 8 Changed 18 months ago by soehms

Replying to SimonKing:

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 18 months ago by vdelecroix

You need two : at the end of this line

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

comment:12 Changed 18 months ago by SimonKing

  • Branch changed from u/soehms/conversion_problem_laurent_fraction_field_26425 to u/SimonKing/conversion_problem_laurent_fraction_field_26425

comment:13 Changed 18 months ago by SimonKing

  • Authors set to Sebastian Oehms
  • Commit changed from 2058e65fdc44784dcd34bc6ed000d0815ef8352a to 38ef90172309bc99d0eade264de9bbe603da2404
  • Reviewers set to 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
        if "padics" in line:
            _ = sys.stdout.write(line)
Expected:
    <li><a class="reference external" href="../reference/padics/sage/rings/padics/tutorial.html#sage-rings-padics-tutorial" title="(in Sage Reference Manual: p-Adics v...)"><span>Introduction to the p-adics</span></a></li>
Got:
    <li><a class="reference external" href="../reference/padics/sage/rings/padics/tutorial.html#sage-rings-padics-tutorial" title="(in Sage Reference Manual: p-Adics v8.6.rc1)"><span>Introduction to the -adics</span></a></li>
**********************************************************************
1 item had failures:
   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.

I added reviewer and author names. Please check for misspellings.


New commits:

38ef901Fix a typo in the docs

comment:14 Changed 18 months ago by SimonKing

  • Status changed from needs_review to 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.
Git branch: HEAD
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
        if "padics" in line:
            _ = sys.stdout.write(line)
Expected:
    <li><a class="reference external" href="../reference/padics/sage/rings/padics/tutorial.html#sage-rings-padics-tutorial" title="(in Sage Reference Manual: p-Adics v...)"><span>Introduction to the p-adics</span></a></li>
Got:
    <li><a class="reference external" href="../reference/padics/sage/rings/padics/tutorial.html#sage-rings-padics-tutorial" title="(in Sage Reference Manual: p-Adics v8.6.rc1)"><span>Introduction to the -adics</span></a></li>
**********************************************************************
1 item had failures:
   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 18 months ago by soehms

Thanks a lot, Simon!

comment:16 Changed 18 months ago by vbraun

  • Branch changed from u/SimonKing/conversion_problem_laurent_fraction_field_26425 to 38ef90172309bc99d0eade264de9bbe603da2404
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.