Redundant minus sign in PolyDict polynomial

[kwankyu]

There is a tiny bug in the polydict implementation of multivariate
polynomial ring.

sage: from sage.rings.polynomial.multi_polynomial_ring import MPolynomialRing_polydict
sage: R.<x,y>=MPolynomialRing_polydict(GF(2),2,order='lex')
sage: R
Multivariate Polynomial Ring in x, y over Finite Field of size 2
sage: f=x+y
sage: f.lt()
-x
sage: f.lm()
-x

The minus sign in "-x" is redundant

Component: basic arithmetic

Author: Kwankyu Lee

Reviewer: Adam Webb

Merged: sage-4.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/7119

[kwankyu]

comment:1

Attachment: trac7119.patch.gz

The patch corrects the bug.

[kwankyu]

Author: Kwankyu Lee

[maxthemouse]

comment:2

I think a doctest should be added for the case that the patch fixes. ~ adam

[kwankyu]

comment:3

Attachment: trac_#7119.patch.gz

The new patch includes doctests and a bugfix of the patch itself.

Martin says:

Alex Ghitza wrote a patch to fix printing of multivariate polynomials in
general

#6551

which might contain your fix. However, it needs some work before it can go in.

However, it seems to me that Alex Ghitza's patch is independent with the current patch.

[kwankyu]

comment:4

The present bug results from the class PolyDict in sage/rings/polynomial/
polydict.pyx rests upon "zero" optional parameter, which defaults to
integer 0, to decide the characteristic of the parent ring when its
object is printed. On the other hand, f.lt() does not set the "zero"
parameter in sage/rings/polynomial/multi_polynomial_element.py.

I think patching the polydict.pyx so as not to rely on the "zero" paramter might be better way to correct the bug. But this is out of my reach.

[maxthemouse]

comment:5

What does the TESTS: label do? When I build the reference the Test section is also included. In which case, why not just add to the Examples section (separated by a line with a :: to start a new section)?

I think it would be easier to use something like:

sage: R.<x,y>=PolynomialRing(GF(2),2,order='lex')
sage: f=x+y
sage: f.lt()
x

Then you don't need the long import statement. What do you think?

Adam

[maxthemouse]

Reviewer: Adam Webb

[maxthemouse]

suggested changes

[maxthemouse]

comment:7

Attachment: trac_7119_b.patch.gz

I added a "suggested changes" patch just to clarify. ~ Adam

[kwankyu]

comment:8

Hi Adam,

The bug is in the polydict engine of multivariate polynomial rings. So your doctest does not check the bug.

About the tests section in the docstring, see this thread in sage-devel: http://groups.google.com/group/sage-devel/browse_frm/thread/2c86e8b59d670502

To summarize, your "suggested changes" should be reverted.

Kwankyu

[maxthemouse]

comment:9

Hi,

That all sounds fine to me. In that case my suggested patch can be ignored. If you know how, you can delete it. In any case, trac_#7119.patch would be the correct patch to apply.

Cheers,

Adam

[mwhansen]

Merged: sage-4.2.alpha0