Opened 4 years ago

Closed 4 years ago

# LaurentPolynomialRing: incorrect conversion of base ring element and failing conversion between rings

Reported by: Owned by: dkrenn major sage-7.6 algebra cheuberg Daniel Krenn Frédéric Chapoton, Travis Scrimshaw N/A 65e2fc5 (Commits) 65e2fc52c62e57cce8ef0b50f1e2b5c2ff57aae6 #21833

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.

### 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:

 ​cbe45b0 `is_constant for laurent polynomials` ​7dc0fbc `conversion to integer/rationals for constant laurent polynomials` ​d1b30ee `test conversion to QQ and ZZ (and refine method definition to allow this)` ​0168003 `minor rewrite wrt to ETuple` ​a809ca5 `splitting helper _split_laurent_polynomial_dict_` ​abe194b `convert multivariate laurent polynomial` ​7660d73 `minor code rewrites in _split_laurent_polynomial_dict_` ​2dc2e98 `docstring of _split_laurent_polynomial_dict_` ​02ae8e7 `conversion of univariate laurent polynomial rings and doctests` ​a5f7483 `rewrite 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:

 ​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/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:

 ​48cb50f `Merge tag '7.5.beta2' into t/21855/laurent-uni-convert`

### comment:8 Changed 4 years ago by git

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

 ​c29aab2 `fix another conversion bug` ​7be4cc3 `rewrite previous bugfix (nicer fix)` ​2ca903a `fix another small bug`

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

• Commit changed from 2ca903a86cc2081aa1199bdb461dfd05709d47ad to 8fa1916db496f3a9ee4c22e4a83e9f66f989ffb1

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

 ​ac104da `cleanup is_constant` ​8fa1916 `simplify _integer_ and _rational_ (using existing methods better)`

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

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

 ​ac104da `cleanup is_constant` ​8fa1916 `simplify _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:

 ​a1b2560 `rewrite 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:

 ​622f22a `correct 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:

 ​5db9ec0 `correct empty dict bug`

### comment:14 Changed 4 years ago by dkrenn

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

### comment:16 follow-ups: ↓ 21 ↓ 22 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:

 ​fded046 `Merge branch 'u/dkrenn/laurent-uni-convert' in 7.5.1` ​c75d370 `trac 21855 some details`

### comment:18 follow-up: ↓ 20 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:

 ​11707ed `trac 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:

 ​65e2fc5 `Merge tag '7.6.beta2' into t/21855/public/21855`

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

does not apply

Merged in 7.6.beta2.

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

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

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: ↓ 25 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

• (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: ↓ 26 Changed 4 years ago by dkrenn

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

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.