Opened 4 years ago

Last modified 7 weeks ago

#17638 needs_work defect

Division of polynomials produces errors when using local orderings

Reported by: enriqueartal Owned by:
Priority: major Milestone: sage-8.4
Component: commutative algebra Keywords: Singular, days94
Cc: SimonKing, jakobkroeker Merged in:
Authors: Mckenzie West Reviewers: ​Luis Felipe Tabera Alonso, Thierry Monteil
Report Upstream: N/A Work issues:
Branch: u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings (Commits) Commit: 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278
Dependencies: Stopgaps:

Description

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.

Change History (23)

comment:1 Changed 3 years ago by SimonKing

  • Cc SimonKing added
  • Component changed from algebraic geometry to commutative algebra

comment:2 Changed 3 years ago by SimonKing

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

comment:3 Changed 3 years ago by enriqueartal

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.

comment:4 Changed 3 years ago by SimonKing

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.

comment:5 Changed 20 months ago by jakobkroeker

  • Cc jakobkroeker added

comment:6 Changed 20 months ago by jakobkroeker

  • Keywords Singular added

comment:7 follow-up: Changed 4 months ago by mwest

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)

comment:8 in reply to: ↑ 7 Changed 4 months ago by tmonteil

Replying to mwest:

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

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

comment:9 Changed 4 months ago by mwest

  • Branch set to u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings

comment:10 Changed 4 months ago by mwest

  • Commit set to 989c4fc2850c173ea9034f22c7742c614840a770
  • Keywords days94 added

New commits:

989c4fcAdded test to verify division works

comment:11 Changed 4 months ago by mwest

  • Status changed from new to needs_review

comment:12 Changed 4 months ago by lftabera

  • Authors set to Mckenzie West
  • Milestone changed from sage-6.5 to sage-8.3

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...

comment:13 Changed 4 months ago by mwest

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

comment:14 Changed 4 months ago by lftabera

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.

comment:15 Changed 4 months ago by git

  • Commit changed from 989c4fc2850c173ea9034f22c7742c614840a770 to b958f3606d4518e240ac942d0502c7c9a3efb2c6

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

b958f36properly formated ::

comment:16 Changed 4 months ago by git

  • Commit changed from b958f3606d4518e240ac942d0502c7c9a3efb2c6 to 703ad58668b598f6276bd4c4dbfe6c69e36ea08c

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

703ad58Removed print function from test

comment:17 Changed 4 months ago by git

  • Commit changed from 703ad58668b598f6276bd4c4dbfe6c69e36ea08c to c2c1e9efd9b678739729fc755e70431e0cdd0f4b

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

c2c1e9eremoved spacing for assignment within function

comment:18 Changed 4 months ago by tmonteil

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 989c4fc2850c173ea9034f22c7742c614840a770 (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.

Last edited 4 months ago by tmonteil (previous) (diff)

comment:19 Changed 4 months ago by git

  • Commit changed from c2c1e9efd9b678739729fc755e70431e0cdd0f4b to 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278

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

8dd8ebecorrecting white spacing

comment:20 Changed 4 months ago by lftabera

  • Reviewers set to ​Luis Felipe Tabera Alonso, Thierry Monteil
  • Status changed from needs_review to positive_review

comment:21 Changed 4 months ago by tmonteil

It is all good with me, and with the newer location, we can see the test when we do:

sage: R.<x,y> = PolynomialRing(QQ, order='neglex')
sage: f = 1 + y
sage: f._div_??
Last edited 4 months ago by tmonteil (previous) (diff)

comment:22 Changed 2 months ago by vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

comment:23 Changed 7 weeks ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

Note: See TracTickets for help on using tickets.