Division of polynomials produces errors when using local orderings

[enriqueartal]

Consider two polynomials f,g in a ring define using a local ordering, e.g.,

R.<x,y>=PolynomialRing(QQ,order='neglex')

If the leading monomial of g is 1 then f/g will produces a wrong result, namely f divided by the independent coefficient of g. Note that in that case g.is_unit() yields True, since for Singular R is not Q[x,y] but the ordering localization (i.e., one can divide by the polynomials whose leading monomial is 1).

PS: It is my first ticket, I apologize for the mistakes.

CC: @simon-king-jena @sagetrac-jakobkroeker @slel

Component: commutative algebra

Keywords: Singular, days94

Author: Mckenzie West

Branch/Commit: 8dd8ebe

Reviewer: Luis Felipe Tabera Alonso, Thierry Monteil

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

[simon-king-jena]

comment:2

Example:

sage: R.<x,y>=PolynomialRing(QQ,order='neglex')
sage: f = 1+y
sage: g = 1+x
sage: f/g
1 + y
sage: _*g==f
False

[enriqueartal]

comment:3

As far as I remember, in the definition of the division of two polynomials, it checks first if the denominator is a unit. If it is, it divides each monomial by the indepent coefficient of the polynomial. There may be two possible solutions. The simple one would be to change this behavior for the division. A more permanent one, which was discussed in this thread proposes to create a new class of localized rings.

[simon-king-jena]

comment:4

Note that this is the code of "div" for multivariate polynomial rings:

        cdef poly *p
        cdef MPolynomial_libsingular right = <MPolynomial_libsingular>right_ringelement
        cdef bint is_field = left._parent._base.is_field()
        if p_IsConstant(right._poly, right._parent_ring):
            if is_field:
                singular_polynomial_div_coeff(&p, left._poly, right._poly, right._parent_ring)
                return new_MP(left._parent, p)
            else:
                return left.change_ring(left.base_ring().fraction_field())/right_ringelement
        else:
            return (left._parent).fraction_field()(left,right_ringelement)

So, it is tested with the libsingular function !p_IsConstant whether the divisor is constant, hence, we can divide by the leading coefficient. I bet !p_IsConstant assumes a positive ordering and just tests whether the leading monomial is of degree zero.

[sagetrac-jakobkroeker]

Changed keywords from none to Singular

[mckenziewest]

comment:7

I checked the example again and it now works, is this fixed?

sage: R.<x,y>=PolynomialRing(QQ,order='neglex')
sage: f = 1+y
sage: g = 1+x
sage: f/g
(y + 1)/(x + 1)

[sagetrac-tmonteil]

comment:8

Replying to @mckenziewest:

I checked the example again and it now works, is this fixed?

In any case, it deserves a dedicated doctest to prevent future regression.

[mckenziewest]

Branch: u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings

[mckenziewest]

Changed keywords from Singular to Singular, days94

[mckenziewest]

Commit: 989c4fc

[mckenziewest]

New commits:

989c4fc

Added test to verify division works

[lftabera]

comment:12

This looks good to me. Note that the _div_ has not chaged.

Just one question, would it better if the test appears in src/sage/rings/polynomial/multi_polynomial_element.py? This is where the code in question is executed...

[lftabera]

Author: Mckenzie West

[mckenziewest]

comment:13

I added the test to the definition of __div__ in src/sage/rings/polynomial/multi_polynomial_element.py
are you thinking somewhere else?

[lftabera]

comment:14

yes, sorry, I was thinking of the method _div_ in
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
which is where the actual _div_ is performed for this ring.

[sagetrac-git]

Changed commit from 989c4fc to b958f36

[sagetrac-git]

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

b958f36

properly formated ::

[sagetrac-git]

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

703ad58

Removed print function from test

[sagetrac-git]

Changed commit from b958f36 to 703ad58

[sagetrac-git]

Changed commit from 703ad58 to c2c1e9e

[sagetrac-git]

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

c2c1e9e

removed spacing for assignment within function

[sagetrac-tmonteil]

comment:18

For reference, there is some documentation about formatting on the developer guide, see http://doc.sagemath.org/html/en/developer/coding_basics.html#python-code-style

In particular, you can add spacing between QQ, and order="neglex", and perhaps between the + (though the current code is already readable).

About the ::, just to make things clearer than our previous discussion:

blah

::

it the secure way to write blah followed by a block of code,

blah ::

is equivalent to the previous one.

However,

blah::

is equivalent to

blah:

::

which is therefore equivalent to:

blah: ::

[My personal preference goes to first and third, but it is up to you]

What was wrong in the commit 989c4fc (comment 10) was

Ensure that :trac:`17638` is fixed.::

wich will print both a dot and a column, but i fear that now you removed both, while it might be good to keep a column.

[slel]

comment:24

Discussed on sage-devel at:

• https://groups.google.com/d/topic/sage-devel/nDoUXqpJ6OU/discussion

[mkoeppe]

comment:26

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:27

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

comment:30

Replying to @vbraun:

Merge conflict

The merge conflict has magically healed by itself.

[vbraun]

comment:32

Merge failure on top of:

341d434082 Trac #33834: Fix map_reduce doctest

40dfa7a1a3 Trac #33828: Fix conda workflow

b90120d1cf Trac #33761: OpenSSL 3.0.3 security update

bdcb741 Trac #33700: Developer's guide: Expand on GitHub accounts and SSH keys

bf6aeb9 Updated SageMath version to 9.6

reviewer u'\u200bLuis Felipe Tabera Alonso, Thierry Monteil' does not look right

[mkoeppe]

Changed reviewer from ​Luis Felipe Tabera Alonso, Thierry Monteil to Luis Felipe Tabera Alonso, Thierry Monteil

[mkoeppe]

comment:34

removed the offending character

[vbraun]

Changed branch from u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings to 8dd8ebe