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:  sage8.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 )
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
 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
comment:2 Changed 2 years ago by
 Description modified (diff)
comment:3 Changed 18 months ago by
 Keywords days100 added
comment:4 Changed 18 months ago by
 Milestone changed from sage8.4 to sage8.9
comment:5 Changed 18 months ago by
 Branch set to u/soehms/conversion_problem_laurent_fraction_field_26425
comment:6 Changed 18 months ago by
 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:
3716151  26425: initial

comment:7 Changed 18 months ago by
 Cc SimonKing added
comment:8 followup: ↓ 10 Changed 18 months 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 18 months ago by
 Commit changed from 37161517cfcc15caf415816cd4dfbd4eda73f0b2 to 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 18 months 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 18 months 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 18 months ago by
 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
 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 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 18 months ago by
 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 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:15 Changed 18 months ago by
Thanks a lot, Simon!
comment:16 Changed 18 months ago by
 Branch changed from u/SimonKing/conversion_problem_laurent_fraction_field_26425 to 38ef90172309bc99d0eade264de9bbe603da2404
 Resolution set to fixed
 Status changed from positive_review to closed
This is not a coercion, but a conversion (as it does not have to be defined going back to
R
).