Opened 4 years ago
Closed 4 years ago
#26425 closed defect (fixed)
Conversion problem between Laurent polynomial ring and its field of fractions
Reported by:  Sebastian Oehms  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  algebra  Keywords:  coercion, laurent plynomial, field of fraction, days100 
Cc:  Travis Scrimshaw, Simon King  Merged in:  
Authors:  Sebastian Oehms  Reviewers:  Simon King, Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  38ef901 (Commits, GitHub, GitLab)  Commit:  38ef90172309bc99d0eade264de9bbe603da2404 
Dependencies:  Stopgaps: 
Description (last modified by )
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 4 years ago by
Description:  modified (diff) 

Summary:  Coercion problem between Laurent polynomial ring and its field of fraction → Conversion problem between Laurent polynomial ring and its field of fractions 
comment:2 Changed 4 years ago by
Description:  modified (diff) 

comment:3 Changed 4 years ago by
Keywords:  days100 added 

comment:4 Changed 4 years ago by
Milestone:  sage8.4 → sage8.9 

comment:5 Changed 4 years ago by
Branch:  → u/soehms/conversion_problem_laurent_fraction_field_26425 

comment:6 Changed 4 years ago by
Commit:  → 37161517cfcc15caf415816cd4dfbd4eda73f0b2 

Status:  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:7 Changed 4 years ago by
Cc:  Simon King added 

comment:8 followup: 10 Changed 4 years ago by
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()
comment:9 Changed 4 years ago by
Commit:  37161517cfcc15caf415816cd4dfbd4eda73f0b2 → 2058e65fdc44784dcd34bc6ed000d0815ef8352a 

Branch pushed to git repo; I updated commit sha1. New commits:
2058e65  26425: correction accordng to reviewers suggestion

comment:10 Changed 4 years ago by
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 4 years ago by
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
Branch:  u/soehms/conversion_problem_laurent_fraction_field_26425 → u/SimonKing/conversion_problem_laurent_fraction_field_26425 

comment:13 Changed 4 years ago by
Authors:  → Sebastian Oehms 

Commit:  2058e65fdc44784dcd34bc6ed000d0815ef8352a → 38ef90172309bc99d0eade264de9bbe603da2404 
Reviewers:  → Simon King, Vincent Delecroix 
I fixed the missing ::
.
All but one tests pass. The failing test is
king@klap:~/Sage/git/sage$ ./sage t warnlong 47.6 src/doc/common/conf.py # 1 doctest failed Running doctests with ID 20190725000110c2c7af32. 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 warnlong 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#sageringspadicstutorial" title="(in Sage Reference Manual: pAdics v...)"><span>Introduction to the padics</span></a></li> Got: <li><a class="reference external" href="../reference/padics/sage/rings/padics/tutorial.html#sageringspadicstutorial" title="(in Sage Reference Manual: pAdics 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 warnlong 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:
38ef901  Fix a typo in the docs

comment:14 Changed 4 years ago by
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 warnlong 47.6 src/doc/common/conf.py Running doctests with ID 2019072500125364443448. 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 warnlong 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#sageringspadicstutorial" title="(in Sage Reference Manual: pAdics v...)"><span>Introduction to the padics</span></a></li> Got: <li><a class="reference external" href="../reference/padics/sage/rings/padics/tutorial.html#sageringspadicstutorial" title="(in Sage Reference Manual: pAdics 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 warnlong 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:16 Changed 4 years ago by
Branch:  u/SimonKing/conversion_problem_laurent_fraction_field_26425 → 38ef90172309bc99d0eade264de9bbe603da2404 

Resolution:  → fixed 
Status:  positive_review → closed 
This is not a coercion, but a conversion (as it does not have to be defined going back to
R
).