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 )
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 sage-8.4 to sage-8.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 follow-up: ↓ 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 --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:
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 --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
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
).