Opened 8 years ago

Closed 17 months ago

# Inconsistency in polynomial .reverse(n)

Reported by: Owned by: defeo minor sage-9.0 commutative algebra polynomial univariate reverse mmezzarobba, jhpalmieri Bruno Grenet Frédéric Chapoton N/A bd32a84 bd32a8489a8e2b13312a08ae021f4b9d328c2226

### Description

The optional argument of the `.reverse()` method of univariate polynomials is interpreted inconsistently through different classes.

Rationals interpret is as "length":

```sage: _.<x> = QQ[]
sage: (x+1).reverse(1)
1
sage: (x).reverse(1)
0
```

The docstring for generic polynomials (inherited by `CC`, number fields, `Polynomial_GF2X`, `Polynomial_ZZ_pEX`, ...) says:

If an optional degree argument is given the coefficient list will be truncated or zero padded as necessary and the reverse polynomial will have the specified degree.

but the behaviour is inconsistent with it

```sage: _.<x> = GF(2)[]
sage: (x+1).reverse(1)
x + 1
sage: (x).reverse(1)
1
sage: ['reverse' in cl.__dict__ for cl in  inspect.getmro(x.__class__)]
[False, False, True, False, False, False, False, False, False, False]
sage: inspect.getmro(x.__class__)[2]
<type 'sage.rings.polynomial.polynomial_element.Polynomial'>
```

`Polynomial_zmod_flint` and `Polynomial_integer_dense_flint` have the exact same docstring and behaviour, though they do not inherit `.reverse()` from the generic class:

```sage: _.<x> = ZZ[]
sage: (x+1).reverse(1)
x + 1
sage: (x).reverse(1)
1
sage: ['reverse' in cl.__dict__ for cl in  inspect.getmro(x.__class__)]
[True, True, False, False, False, False, False, False, False]
<type 'sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint'>
```

Reals take no optional argument. The docstring says

Returns x^d f(1/x) where d is the degree of f.

and the behaviour is consistent with it

```sage: (x+1).reverse()
x + 1.00000000000000
sage: x.reverse()
1.00000000000000
```

In my opinion the best behaviour is the one of the generic class, but the docstring should be amended to something similar to the last one, which is the proper mathematical definition. The behaviour of rationals should be corrected to conform to the other classes.

### comment:1 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:3 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:4 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:6 Changed 2 years ago by chapoton

• Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
• Status changed from new to needs_review

This is obsolete. `QQ['x']` now works as the other cases.

### comment:7 Changed 2 years ago by jhpalmieri

There remain two issues: unifying the documentation, and also the version for `RR` does not allow for an optional argument. Low priority, but I say we keep this open.

### comment:8 Changed 20 months ago by bruno

• Authors set to Bruno Grenet
• Branch set to u/bruno/15077_reverse_consistency
• Milestone changed from sage-duplicate/invalid/wontfix to sage-8.9

There remained actually more issues. The issue for `QQ['x']` was indeed fixed by ticket #21194 but that ticket only fixed some of the inconsistencies. Those that I have noticed and fix are:

• No `degree` parameter for polynomials over `RR` (as already mentioned);
• No `degree` parameter for polynomials over `Zmod(...)` using NTL;
• Several inconsistencies in the documentation (polynomials over `ZZ` and `Zmod(...)` using Flint, polynomials over p-adics).

I hope I didn't forgot anything!

### comment:9 Changed 20 months ago by git

• Commit set to bd32a8489a8e2b13312a08ae021f4b9d328c2226

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

 ​64c41ef `15077: reverse for real polynomial with optional parameter` ​1fc2a6e `15077: Update docstring+tests for Flint (integer & zmod)` ​7fec024 `15077: Make reverse consistent for NTL rings` ​bd32a84 `15077: Make reverse for poly over padics consistent with other implementations`

### comment:10 Changed 17 months ago by chapoton

• Reviewers set to Frédéric Chapoton
• Status changed from needs_review to positive_review

ok, let it be

### comment:11 Changed 17 months ago by chapoton

• Milestone changed from sage-8.9 to sage-9.0

### comment:12 Changed 17 months ago by vbraun

• Branch changed from u/bruno/15077_reverse_consistency to bd32a8489a8e2b13312a08ae021f4b9d328c2226
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.