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: sage-7.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 dkrenn)

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
  1. (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)
    
  2. 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 dkrenn

  • Description modified (diff)

comment:2 Changed 4 years ago by dkrenn

  • Branch set to u/dkrenn/laurent-uni-convert

comment:3 Changed 4 years ago by dkrenn

  • Authors set to Daniel Krenn
  • 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

Let's see what the patchbot says...


Last 10 new commits:

cbe45b0is_constant for laurent polynomials
7dc0fbcconversion to integer/rationals for constant laurent polynomials
d1b30eetest conversion to QQ and ZZ (and refine method definition to allow this)
0168003minor rewrite wrt to ETuple
a809ca5splitting helper _split_laurent_polynomial_dict_
abe194bconvert multivariate laurent polynomial
7660d73minor code rewrites in _split_laurent_polynomial_dict_
2dc2e98docstring of _split_laurent_polynomial_dict_
02ae8e7conversion of univariate laurent polynomial rings and doctests
a5f7483rewrite parts of the element construction to handle conversions to the base ring better

comment:4 Changed 4 years ago by dkrenn

  • Description modified (diff)

comment:5 Changed 4 years ago by git

  • Commit changed from a5f7483c13c27be416db6155028dc77089f0b9d3 to 8dba3f5d304132f17e206c4c5a9f1d0712e07110

Branch pushed to git repo; I updated commit sha1. New commits:

ef8a412implement LaurentPolynomialRing.variable_names_recursive
9e1c95fadapt docstring of PolynomialRing.variable_names_recursive
a6f56d3laurent polynomial converter from symbolic expressions
7226856adapt docstring
8b4c684method Expression.laurent_polynomial
ba5f230create laurent polynomials from symbolic expressions (in LaurentPolynomialRing)
8dba3f5Merge branch 'u/dakrenn/laurent-symbolic' into u/dkrenn/laurent-uni-convert

comment:6 Changed 4 years ago by dkrenn

  • Dependencies set to #21833

comment:7 Changed 4 years ago by git

  • Commit changed from 8dba3f5d304132f17e206c4c5a9f1d0712e07110 to 48cb50f86047adb633a3a7edf061fabd6c1bf4a6

Branch pushed to git repo; I updated commit sha1. New commits:

48cb50fMerge tag '7.5.beta2' into t/21855/laurent-uni-convert

comment:8 Changed 4 years ago by git

  • Commit changed from 48cb50f86047adb633a3a7edf061fabd6c1bf4a6 to 2ca903a86cc2081aa1199bdb461dfd05709d47ad

Branch pushed to git repo; I updated commit sha1. New commits:

c29aab2fix another conversion bug
7be4cc3rewrite previous bugfix (nicer fix)
2ca903afix another small bug

comment:9 follow-up: Changed 4 years ago by git

  • Commit changed from 2ca903a86cc2081aa1199bdb461dfd05709d47ad to 8fa1916db496f3a9ee4c22e4a83e9f66f989ffb1

Branch pushed to git repo; I updated commit sha1. New commits:

ac104dacleanup is_constant
8fa1916simplify _integer_ and _rational_ (using existing methods better)

comment:10 in reply to: ↑ 9 Changed 4 years ago by dkrenn

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

ac104dacleanup is_constant
8fa1916simplify _integer_ and _rational_ (using existing methods better)

Some code simplifications.

comment:11 Changed 4 years ago by git

  • Commit changed from 8fa1916db496f3a9ee4c22e4a83e9f66f989ffb1 to a1b256017e411d2a39485f5556df8d4219dc69f2

Branch pushed to git repo; I updated commit sha1. New commits:

a1b2560rewrite of is_constant

comment:12 Changed 4 years ago by git

  • Commit changed from a1b256017e411d2a39485f5556df8d4219dc69f2 to 622f22a0e92247df8ade150f5282c1030e598137

Branch pushed to git repo; I updated commit sha1. New commits:

622f22acorrect another small bug

comment:13 Changed 4 years ago by git

  • Commit changed from 622f22a0e92247df8ade150f5282c1030e598137 to 5db9ec0f942bcabcc7cd51dd1a6fa54abdc3d5fa

Branch pushed to git repo; I updated commit sha1. New commits:

5db9ec0correct empty dict bug

comment:14 Changed 4 years ago by dkrenn

Two more bug-fixes...ticket needs review now.

comment:15 Changed 4 years ago by cheuberg

  • Cc cheuberg added

comment:16 follow-ups: Changed 4 years ago by 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 mixin-class)? Or are there some subtle differences that I am not quite considering? I feel like this could make code maintenance easier in the long-run.

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 chapoton

  • Branch changed from u/dkrenn/laurent-uni-convert to public/21855
  • Commit changed from 5db9ec0f942bcabcc7cd51dd1a6fa54abdc3d5fa to c75d370bcd35b708f04456dc2290fdd4222089eb

did some partial work on Travis comments


New commits:

fded046Merge branch 'u/dkrenn/laurent-uni-convert' in 7.5.1
c75d370trac 21855 some details

comment:18 follow-up: Changed 4 years ago by chapoton

  • Commit changed from c75d370bcd35b708f04456dc2290fdd4222089eb to 11707ed48e5445aff097b69bbc147c83184ba98e
  • Status changed from needs_review to needs_work

does not apply


New commits:

11707edtrac 21855 more details

comment:19 Changed 4 years ago by git

  • Commit changed from 11707ed48e5445aff097b69bbc147c83184ba98e to 65e2fc52c62e57cce8ef0b50f1e2b5c2ff57aae6

Branch pushed to git repo; I updated commit sha1. New commits:

65e2fc5Merge tag '7.6.beta2' into t/21855/public/21855

comment:20 in reply to: ↑ 18 Changed 4 years ago by dkrenn

Replying to chapoton:

does not apply

Merged in 7.6.beta2.

comment:21 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by dkrenn

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 dkrenn

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 mixin-class)? Or are there some subtle differences that I am not quite considering? I feel like this could make code maintenance easier in the long-run.

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 dkrenn

  • Status changed from needs_work to needs_review

comment:24 in reply to: ↑ 21 ; follow-up: Changed 4 years ago by tscrim

  • Milestone changed from sage-7.5 to sage-7.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 ; follow-up: Changed 4 years ago by 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 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 tscrim

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 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.

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 like The 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 vbraun

  • Branch changed from public/21855 to 65e2fc52c62e57cce8ef0b50f1e2b5c2ff57aae6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.