Opened 4 years ago
Closed 4 years ago
#24431 closed enhancement (fixed)
Fix coercions and pushout for Laurent series
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  algebra  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  8abc33b (Commits, GitHub, GitLab)  Commit:  8abc33b3dd7001c7022e9c114563251d7e668304 
Dependencies:  #24420  Stopgaps: 
Description (last modified by )
There should be an automatic extension of scalars in the following situation
sage: R.<x> = LaurentSeriesRing(QQ) sage: QQbar.gen() * x Traceback (most recent call last): ... TypeError: unsupported operand parent(s) for *: 'Algebraic Field' and 'Laurent Series Ring in x over Rational Field'
To be compared with
sage: R.<x> = PowerSeriesRing(QQ) sage: QQbar.gen() * x I*x sage: parent(_) Power Series Ring in x over Algebraic Field
Currently, these pushouts are allowed:
sage: x = LaurentSeriesRing(QQ, 'x').gen() sage: y = PolynomialRing(QQ, 'y').gen() sage: x * y x*y sage: parent(x * y) Univariate Polynomial Ring in y over Laurent Series Ring in x over Rational Field
This is inconsistent with PowerSeriesRing
.
By fixing these problems, we had to adapt some code in sage/schemes/hyperelliptic_curves/hyperelliptic_generic.py
that was using the aforementioned pushout.
Change History (23)
comment:1 Changed 4 years ago by
 Branch set to u/vdelecroix/24431
 Commit set to eb2bba461ab15f7cbb21733d42797af8d17f3464
 Component changed from PLEASE CHANGE to algebra
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit changed from eb2bba461ab15f7cbb21733d42797af8d17f3464 to 27f324eddb758edbeaeee37fde25dfd74e94d888
comment:3 Changed 4 years ago by
 Commit changed from 27f324eddb758edbeaeee37fde25dfd74e94d888 to 00deb276b62aaadc5d1a77a1965821e3efb2def6
comment:4 Changed 4 years ago by
 Commit changed from 00deb276b62aaadc5d1a77a1965821e3efb2def6 to c905ee96cc6f80e679cc55ddaf9e02380e18672b
comment:5 Changed 4 years ago by
 Commit changed from c905ee96cc6f80e679cc55ddaf9e02380e18672b to 00deb276b62aaadc5d1a77a1965821e3efb2def6
comment:6 Changed 4 years ago by
 Description modified (diff)
 Status changed from needs_review to needs_work
 Summary changed from Pushout for Laurent series to Fix coercions and pushout for Laurent series
comment:7 Changed 4 years ago by
 Commit changed from 00deb276b62aaadc5d1a77a1965821e3efb2def6 to 1b1bbbc062f24af9adecd11ddab5a12bf8812636
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1b1bbbc  24431: Laurent series pushout and coercions

comment:8 Changed 4 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:9 Changed 4 years ago by
 Commit changed from 1b1bbbc062f24af9adecd11ddab5a12bf8812636 to b3f11d9d244e72085a96d64e1286aa43f70df44f
comment:10 Changed 4 years ago by
rebased on 8.2.beta2
comment:11 followup: ↓ 12 Changed 4 years ago by
LGTM except I don't understand the nonimport changes in schemes/hyperelliptic_curves/hyperelliptic_generic.py
. (Also, is LaurentPolynomialRing
is not used in that file.)
comment:12 in reply to: ↑ 11 Changed 4 years ago by
Replying to tscrim:
LGTM except I don't understand the nonimport changes in
schemes/hyperelliptic_curves/hyperelliptic_generic.py
. (Also, isLaurentPolynomialRing
is not used in that file.)
I believe you refer to this line
 L = PolynomialRing(self.base_ring(),'x') + L = PolynomialRing(K,'x')
This is the third point in the ticket description.
comment:13 Changed 4 years ago by
 Description modified (diff)
 Reviewers set to Travis Scrimshaw
I understand now the reason why that change is needed. Thanks. (Although I don't quite understand why that pushout is no longer allowed, but that's okay.)
Just remove the unneeded import of LaurentPolynomialRing
and the \
's here
@@ 410,7 +565,11 @@ class LaurentSeriesRing_generic(UniqueRepresentation, ring.CommutativeRing): from sage.rings.polynomial.polynomial_ring import is_PolynomialRing from sage.rings.power_series_ring import is_PowerSeriesRing  if ((is_LaurentSeriesRing(P) or is_PowerSeriesRing(P) or is_PolynomialRing(P)) + from sage.rings.polynomial.laurent_polynomial_ring import is_LaurentPolynomialRing + if ((is_LaurentSeriesRing(P) or \ + is_LaurentPolynomialRing(P) or \ + is_PowerSeriesRing(P) or \ + is_PolynomialRing(P)) and P.variable_name() == self.variable_name() and A.has_coerce_map_from(P.base_ring())): return True
and you can set a positive review on my behalf.
comment:14 Changed 4 years ago by
 Commit changed from b3f11d9d244e72085a96d64e1286aa43f70df44f to de63301a78c5746c7d3384a19bac1a4e7564741c
Branch pushed to git repo; I updated commit sha1. New commits:
de63301  24431: unneeded '\' and more tests

comment:15 Changed 4 years ago by
I did not find the import you were talking about in 13.
comment:16 Changed 4 years ago by

src/sage/schemes/hyperelliptic_curves/hyperelliptic_generic.py
diff git a/src/sage/schemes/hyperelliptic_curves/hyperelliptic_generic.py b/src/sage/schemes/hyperelliptic_curves/hyperelliptic_generic.py index cdca282..9e9897a 100644
a b from __future__ import absolute_import 33 33 # http://www.gnu.org/licenses/ 34 34 #***************************************************************************** 35 35 36 from sage.rings.all import PolynomialRing, RR, PowerSeriesRing, LaurentSeriesRing, O 36 from sage.rings.polynomial.all import PolynomialRing, LaurentPolynomialRing 37 from sage.rings.big_oh import O 38 from sage.rings.power_series_ring import PowerSeriesRing 39 from sage.rings.laurent_series_ring import LaurentSeriesRing 40 from sage.rings.real_mpfr import RR 37 41 from sage.functions.all import log 38 42 from sage.structure.category_object import normalize_names 39 43
comment:17 Changed 4 years ago by
 Commit changed from de63301a78c5746c7d3384a19bac1a4e7564741c to cd4b485c31009875acf25e8082477b2da7288ee5
Branch pushed to git repo; I updated commit sha1. New commits:
cd4b485  24431: remove unused import

comment:18 Changed 4 years ago by
 Status changed from needs_review to needs_work
Looks like there is a test failure since 8.2.beta3.
comment:19 Changed 4 years ago by
It is trivial due to the output of the functor. Once fixed, you can set a positive review on my behalf.
comment:20 Changed 4 years ago by
 Commit changed from cd4b485c31009875acf25e8082477b2da7288ee5 to 8abc33b3dd7001c7022e9c114563251d7e668304
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8abc33b  24431: Laurent series pushout and coercions

comment:21 Changed 4 years ago by
 Status changed from needs_work to needs_review
Rebased on beta4. Waiting for patchbots as an extra check.
comment:22 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:23 Changed 4 years ago by
 Branch changed from u/vdelecroix/24431 to 8abc33b3dd7001c7022e9c114563251d7e668304
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
24420: clean laurent series
24431: pushout for Laurent series