Opened 4 years ago
Closed 4 years ago
#21855 closed defect (fixed)
LaurentPolynomialRing: incorrect conversion of base ring element and failing conversion between rings
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  algebra  Keywords:  
Cc:  cheuberg  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Frédéric Chapoton, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  65e2fc5 (Commits)  Commit:  65e2fc52c62e57cce8ef0b50f1e2b5c2ff57aae6 
Dependencies:  #21833  Stopgaps: 
Description (last modified by )
1.
sage: P = LaurentPolynomialRing(QQ, 'a, b') sage: Q = LaurentPolynomialRing(P, 'c, d') sage: Q(P.0) c
but the expected result is a
as it is with polynomial rings:
sage: P = PolynomialRing(QQ, 'a, b') sage: Q = PolynomialRing(P, 'c, d') sage: Q(P.0) a
 (In some sense) more generally, conversion between certain laurent polynomial rings (e.g. isomorphic rings) fail. E.g. the following should work
sage: L.<a, b, c, d> = LaurentPolynomialRing(QQ) sage: M = LaurentPolynomialRing(QQ, 'c, d') sage: Mc, Md = M.gens() sage: N = LaurentPolynomialRing(M, 'a, b') sage: Na, Nb = N.gens() sage: M(c/d) sage: N(a*b/c/d) sage: N(c/d) sage: L(Mc) sage: L(Nb)
 As needed to fix the above, a method
is_constant
, as well as methods for the conversions of constant laurent polynomials are to be implemented.
Change History (27)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 4 years ago by
 Branch set to u/dkrenn/laurentuniconvert
comment:3 Changed 4 years ago by
 Commit set to a5f7483c13c27be416db6155028dc77089f0b9d3
 Description modified (diff)
 Status changed from new to needs_review
 Summary changed from LaurentPolynomialRing: incorrect conversion of base ring element to LaurentPolynomialRing: incorrect conversion of base ring element and failing conversion between rings
comment:4 Changed 4 years ago by
 Description modified (diff)
comment:5 Changed 4 years ago by
 Commit changed from a5f7483c13c27be416db6155028dc77089f0b9d3 to 8dba3f5d304132f17e206c4c5a9f1d0712e07110
Branch pushed to git repo; I updated commit sha1. New commits:
ef8a412  implement LaurentPolynomialRing.variable_names_recursive

9e1c95f  adapt docstring of PolynomialRing.variable_names_recursive

a6f56d3  laurent polynomial converter from symbolic expressions

7226856  adapt docstring

8b4c684  method Expression.laurent_polynomial

ba5f230  create laurent polynomials from symbolic expressions (in LaurentPolynomialRing)

8dba3f5  Merge branch 'u/dakrenn/laurentsymbolic' into u/dkrenn/laurentuniconvert

comment:6 Changed 4 years ago by
 Dependencies set to #21833
comment:7 Changed 4 years ago by
 Commit changed from 8dba3f5d304132f17e206c4c5a9f1d0712e07110 to 48cb50f86047adb633a3a7edf061fabd6c1bf4a6
Branch pushed to git repo; I updated commit sha1. New commits:
48cb50f  Merge tag '7.5.beta2' into t/21855/laurentuniconvert

comment:8 Changed 4 years ago by
 Commit changed from 48cb50f86047adb633a3a7edf061fabd6c1bf4a6 to 2ca903a86cc2081aa1199bdb461dfd05709d47ad
comment:9 followup: ↓ 10 Changed 4 years ago by
 Commit changed from 2ca903a86cc2081aa1199bdb461dfd05709d47ad to 8fa1916db496f3a9ee4c22e4a83e9f66f989ffb1
comment:10 in reply to: ↑ 9 Changed 4 years ago by
comment:11 Changed 4 years ago by
 Commit changed from 8fa1916db496f3a9ee4c22e4a83e9f66f989ffb1 to a1b256017e411d2a39485f5556df8d4219dc69f2
Branch pushed to git repo; I updated commit sha1. New commits:
a1b2560  rewrite of is_constant

comment:12 Changed 4 years ago by
 Commit changed from a1b256017e411d2a39485f5556df8d4219dc69f2 to 622f22a0e92247df8ade150f5282c1030e598137
Branch pushed to git repo; I updated commit sha1. New commits:
622f22a  correct another small bug

comment:13 Changed 4 years ago by
 Commit changed from 622f22a0e92247df8ade150f5282c1030e598137 to 5db9ec0f942bcabcc7cd51dd1a6fa54abdc3d5fa
Branch pushed to git repo; I updated commit sha1. New commits:
5db9ec0  correct empty dict bug

comment:14 Changed 4 years ago by
Two more bugfixes...ticket needs review now.
comment:15 Changed 4 years ago by
 Cc cheuberg added
comment:16 followups: ↓ 21 ↓ 22 Changed 4 years ago by
Since (multivariate) polynomial and Laurent polynomial rings should have essentially the same coercions, how difficult would it be to pull out the code to find the coercions into standalone functions (or as a mixinclass)? Or are there some subtle differences that I am not quite considering? I feel like this could make code maintenance easier in the longrun.
Some other small things:
 Please make sure your
INPUT
blocks are in the proper format (sorry to sound like a broken record)  (Short) Error messages should not have periods.
 It is with a capital letter "Laurent".
 This is a cleaner way of doing multiline statements
return (self._mon == ETuple({}, int(self.parent().ngens())) and self._poly.is_constant())
comment:17 Changed 4 years ago by
 Branch changed from u/dkrenn/laurentuniconvert to public/21855
 Commit changed from 5db9ec0f942bcabcc7cd51dd1a6fa54abdc3d5fa to c75d370bcd35b708f04456dc2290fdd4222089eb
comment:18 followup: ↓ 20 Changed 4 years ago by
 Commit changed from c75d370bcd35b708f04456dc2290fdd4222089eb to 11707ed48e5445aff097b69bbc147c83184ba98e
 Status changed from needs_review to needs_work
comment:19 Changed 4 years ago by
 Commit changed from 11707ed48e5445aff097b69bbc147c83184ba98e to 65e2fc52c62e57cce8ef0b50f1e2b5c2ff57aae6
Branch pushed to git repo; I updated commit sha1. New commits:
65e2fc5  Merge tag '7.6.beta2' into t/21855/public/21855

comment:20 in reply to: ↑ 18 Changed 4 years ago by
comment:21 in reply to: ↑ 16 ; followup: ↓ 24 Changed 4 years ago by
Replying to tscrim:
Some other small things:
 Please make sure your
INPUT
blocks are in the proper format (sorry to sound like a broken record)
chapoton thanks for doing this.
 (Short) Error messages should not have periods.
I am for a correct punctation. Thus if it is a proper English sentence (short or longer), there should be a period. E.g.
ValueError: a is not constant.
Thus either we (meaning I) add the period again or we rewrite this to a not constant
. What do you prefer? (FYI, I prefer the former as it sound "more round".)
 It is with a capital letter "Laurent".
Thank you for doing.
 This is a cleaner way of doing multiline statements
return (self._mon == ETuple({}, int(self.parent().ngens())) and self._poly.is_constant())
Ok, I am fine with it.
comment:22 in reply to: ↑ 16 Changed 4 years ago by
Replying to tscrim:
Since (multivariate) polynomial and Laurent polynomial rings should have essentially the same coercions, how difficult would it be to pull out the code to find the coercions into standalone functions (or as a mixinclass)? Or are there some subtle differences that I am not quite considering? I feel like this could make code maintenance easier in the longrun.
I agree that this is desirable. As I am not that familiar with the polynomial ring code (and it will need some effort to do), I have put this into the follow up ticket #22333.
comment:23 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:24 in reply to: ↑ 21 ; followup: ↓ 25 Changed 4 years ago by
 Milestone changed from sage7.5 to sage7.6
 Reviewers set to Frédéric Chapoton, Travis Scrimshaw
 Status changed from needs_review to positive_review
Replying to dkrenn:
Replying to tscrim:
 (Short) Error messages should not have periods.
I am for a correct punctation. Thus if it is a proper English sentence (short or longer), there should be a period. E.g.
ValueError: a is not constant.Thus either we (meaning I) add the period again or we rewrite this to
a not constant
. What do you prefer? (FYI, I prefer the former as it sound "more round".)
The short version is that this is a Python convention that we (should) try to adhere to. The more technical reason: while they could be considered full sentences, they technically don't begin with a capital letter (the type of error is taken more as a bullet point).
LGTM. Thanks.
comment:25 in reply to: ↑ 24 ; followup: ↓ 26 Changed 4 years ago by
Replying to tscrim:
The short version is that this is a Python convention that we (should) try to adhere to.
Is this really so? I have not found this convention, so it would be great if you could provide a link etc.
By just looking what Python does, it seems not to be clear, what the rule is: E.g. rational division by zero
(which is produced by 1/int(0)
) could also mean "use correct punctation and capitalization", i.e. if not a proper English sentence start small and end with .
, and that does not exclude, if a proper English sentence, then start capitalized and end with period.
The more technical reason: while they could be considered full sentences, they technically don't begin with a capital letter (the type of error is taken more as a bullet point).
So, this means a is not constant.
should not start with a formula, but something like The input a is not constant.
should be fine. ;)
LGTM. Thanks.
Ok, thank you; then let it be as it is for this ticket (However, I'd really like to see a reference for the above.)
comment:26 in reply to: ↑ 25 Changed 4 years ago by
Replying to dkrenn:
Replying to tscrim:
The short version is that this is a Python convention that we (should) try to adhere to.
Is this really so? I have not found this convention, so it would be great if you could provide a link etc.
By just looking what Python does, it seems not to be clear, what the rule is: E.g.
rational division by zero
(which is produced by1/int(0)
) could also mean "use correct punctation and capitalization", i.e. if not a proper English sentence start small and end with.
, and that does not exclude, if a proper English sentence, then start capitalized and end with period.
I don't think this is officially documented, but is just something all Python error messages use. It is also a convention most error messages in Sage use as well.
The more technical reason: while they could be considered full sentences, they technically don't begin with a capital letter (the type of error is taken more as a bullet point).
So, this means
a is not constant.
should not start with a formula, but something likeThe input a is not constant.
should be fine. ;)
Good math papers should never start sentences with formulas. :P We can also always change sentences into clauses too. ;)
comment:27 Changed 4 years ago by
 Branch changed from public/21855 to 65e2fc52c62e57cce8ef0b50f1e2b5c2ff57aae6
 Resolution set to fixed
 Status changed from positive_review to closed
Let's see what the patchbot says...
Last 10 new commits:
is_constant for laurent polynomials
conversion to integer/rationals for constant laurent polynomials
test conversion to QQ and ZZ (and refine method definition to allow this)
minor rewrite wrt to ETuple
splitting helper _split_laurent_polynomial_dict_
convert multivariate laurent polynomial
minor code rewrites in _split_laurent_polynomial_dict_
docstring of _split_laurent_polynomial_dict_
conversion of univariate laurent polynomial rings and doctests
rewrite parts of the element construction to handle conversions to the base ring better