Opened 8 years ago

Closed 7 months ago

# Division of polynomials produces errors when using local orderings

Reported by: Owned by: Enrique Artal Bartolo major sage-9.7 commutative algebra Singular, days94 Simon King, Jakob Kroeker, Samuel Lelièvre Mckenzie West Luis Felipe Tabera Alonso, Thierry Monteil N/A 8dd8ebe 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278

### Description (last modified by Samuel Lelièvre)

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.

### comment:1 Changed 7 years ago by Simon King

Cc: Simon King added algebraic geometry → commutative algebra

### comment:2 Changed 7 years ago by Simon King

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 7 years ago by Enrique Artal Bartolo

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 7 years ago by Simon King

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 6 years ago by Jakob Kroeker

Cc: Jakob Kroeker added

### comment:6 Changed 6 years ago by Jakob Kroeker

Keywords: Singular added

### comment:7 follow-up:  8 Changed 4 years ago by Mckenzie West

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 years ago by Thierry Monteil

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 years ago by Mckenzie West

Branch: → u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings

### comment:10 Changed 4 years ago by Mckenzie West

Commit: → 989c4fc2850c173ea9034f22c7742c614840a770 days94 added

New commits:

 ​989c4fc `Added test to verify division works`

### comment:11 Changed 4 years ago by Mckenzie West

Status: new → needs_review

### comment:12 Changed 4 years ago by Luis Felipe Tabera Alonso

Authors: → Mckenzie West sage-6.5 → 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 years ago by Mckenzie West

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 years ago by Luis Felipe Tabera Alonso

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 years ago by git

Commit: 989c4fc2850c173ea9034f22c7742c614840a770 → b958f3606d4518e240ac942d0502c7c9a3efb2c6

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

 ​b958f36 `properly formated ::`

### comment:16 Changed 4 years ago by git

Commit: b958f3606d4518e240ac942d0502c7c9a3efb2c6 → 703ad58668b598f6276bd4c4dbfe6c69e36ea08c

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

 ​703ad58 `Removed print function from test`

### comment:17 Changed 4 years ago by git

Commit: 703ad58668b598f6276bd4c4dbfe6c69e36ea08c → c2c1e9efd9b678739729fc755e70431e0cdd0f4b

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

 ​c2c1e9e `removed spacing for assignment within function`

### comment:18 Changed 4 years ago by Thierry Monteil

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 years ago by Thierry Monteil (previous) (diff)

### comment:19 Changed 4 years ago by git

Commit: c2c1e9efd9b678739729fc755e70431e0cdd0f4b → 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278

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

 ​8dd8ebe `correcting white spacing`

### comment:20 Changed 4 years ago by Luis Felipe Tabera Alonso

Reviewers: → ​Luis Felipe Tabera Alonso, Thierry Monteil needs_review → positive_review

### comment:21 Changed 4 years ago by Thierry Monteil

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 years ago by Thierry Monteil (previous) (diff)

### comment:22 Changed 4 years ago by Vincent Delecroix

Milestone: sage-8.3 → sage-8.4

update milestone 8.3 -> 8.4

### comment:23 follow-up:  30 Changed 4 years ago by Volker Braun

Status: positive_review → needs_work

Merge conflict

### comment:24 Changed 3 years ago by Samuel Lelièvre

Cc: Samuel Lelièvre added modified (diff) sage-8.4 → sage-9.2

### comment:25 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2 → sage-9.3

### comment:26 Changed 22 months ago by Matthias Köppe

Milestone: sage-9.3 → sage-9.4

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

### comment:27 Changed 17 months ago by Matthias Köppe

Milestone: sage-9.4 → sage-9.5

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

### comment:28 Changed 12 months ago by Matthias Köppe

Milestone: sage-9.5 → sage-9.6

### comment:29 Changed 7 months ago by Matthias Köppe

Milestone: sage-9.6 → sage-9.7

### comment:30 in reply to:  23 Changed 7 months ago by Matthias Köppe

Replying to vbraun:

Merge conflict

The merge conflict has magically healed by itself.

### comment:31 Changed 7 months ago by Matthias Köppe

Status: needs_work → positive_review

### comment:32 Changed 7 months ago by Volker Braun

Status: positive_review → needs_work

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

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

bf6aeb906d Updated SageMath version to 9.6

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

### comment:33 Changed 7 months ago by Matthias Köppe

Reviewers: ​Luis Felipe Tabera Alonso, Thierry Monteil → Luis Felipe Tabera Alonso, Thierry Monteil needs_work → positive_review

### comment:34 Changed 7 months ago by Matthias Köppe

removed the offending character

### comment:35 Changed 7 months ago by Volker Braun

Branch: u/mwest/division_of_polynomials_produces_errors_when_using_local_orderings → 8dd8ebef54d67ec3f7aae96be0cfaa8392ce9278 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.