Opened 8 years ago
Closed 17 months ago
#15077 closed defect (fixed)
Inconsistency in polynomial .reverse(n)
Reported by:  defeo  Owned by:  

Priority:  minor  Milestone:  sage9.0 
Component:  commutative algebra  Keywords:  polynomial univariate reverse 
Cc:  mmezzarobba, jhpalmieri  Merged in:  
Authors:  Bruno Grenet  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  bd32a84 (Commits, GitHub, GitLab)  Commit:  bd32a8489a8e2b13312a08ae021f4b9d328c2226 
Dependencies:  Stopgaps: 
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.
Change History (12)
comment:1 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:2 Changed 7 years ago by
 Cc mmezzarobba added
comment:3 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:4 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:5 Changed 5 years ago by
 Stopgaps set to inconsistentAnswer
comment:6 Changed 2 years ago by
 Cc jhpalmieri added
 Milestone changed from sage6.4 to sageduplicate/invalid/wontfix
 Status changed from new to needs_review
comment:7 Changed 2 years ago by
 Stopgaps inconsistentAnswer deleted
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
 Branch set to u/bruno/15077_reverse_consistency
 Milestone changed from sageduplicate/invalid/wontfix to sage8.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 overRR
(as already mentioned);  No
degree
parameter for polynomials overZmod(...)
using NTL;  Several inconsistencies in the documentation (polynomials over
ZZ
andZmod(...)
using Flint, polynomials over padics).
I hope I didn't forgot anything!
comment:9 Changed 20 months ago by
 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
 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
 Milestone changed from sage8.9 to sage9.0
comment:12 Changed 17 months ago by
 Branch changed from u/bruno/15077_reverse_consistency to bd32a8489a8e2b13312a08ae021f4b9d328c2226
 Resolution set to fixed
 Status changed from positive_review to closed
This is obsolete.
QQ['x']
now works as the other cases.