Fixes and improvements to EuclideanRings.ParentMethods._test_quo_rem and quo_rem for fields

[nthiery]

This ticket:

• Fixes EuclideanRings.ParentMethods._test_quo_rem to use an not is_zero instead of !=0. This is more consistent and does not
  require coercion.

• Improves EuclideanRings.ParentMethods._test_quo_rem to check that
  the results are in the same parent

• Fixes quo_rem for fields to return the zero of the field instead of 0

• Makes the two implementations FieldElement.quo_rem and
  Fields.ElementMethods.quo_rem consistent and reference each other.

Depends on #19937

CC: @slel @videlec

Component: categories

Keywords: days70.5

Author: Nicolas M. Thiéry

Branch/Commit: u/nthiery/fix_euclideanrings_parentmethods__test_quo_rem_to_use__not_r_is_zero____rather_than__r__0_ @ eda4420

Reviewer: Jeroen Demeyer, Samuel Lelièvre

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

[nthiery]

Branch: u/nthiery/fix_euclideanrings_parentmethods__test_quo_rem_to_use__not_r_is_zero____rather_than__r__0_

[nthiery]

New commits:

db4d31e	19937: Implement generic `__div__` and `_div_` in the categories
de58229	19937: Documented that division may fail even if right divides left
2d11e11	19939: Fix EuclideanRings.ParentMethods._test_quo_rem to use `not r.is_zero()` rather than `r!=0`

[nthiery]

Reviewer: Samuel Lelièvre

[nthiery]

Author: Nicolas M. Thiéry

[nthiery]

Commit: 2d11e11

[nthiery]

Dependencies: #19937

[jdemeyer]

comment:4

Trivial comment, but why not replace

if r != 0:

by

if r:

If this works, this will be the fastest way.

[nthiery]

comment:5

I indered pondered about this. I went for using r.is_zero() for
style consistency with the nearby zero tests. But I don't have a
strong opinion, and I am fine switching to if r. In that case, maybe
the nearby zero tests should be changed accordingly for
consistency. What do you think?

By the way: I looked at the current tests failure. They are due to the
default implementation of quo_rem for Fields which return a plain
0 for the remainder instead of the zero of the field:

    sage: K.<a> = NumberField(x^3 - 2); K
    Number Field in a with defining polynomial x^3 - 2
    sage: y = K.an_element()
    sage: y.parent()
    Number Field in a with defining polynomial x^3 - 2
    sage: q,r = y.quo_rem(y-1)
    sage: q,r
    sage: parent(q)               # OK
    Number Field in a with defining polynomial x^3 - 2
    sage: type(r)                 # GASP!!!
    <type 'int'>

I am going to handle this right now by:

1. Updating EuclideanRings.ParentMethods._test_quo_rem to test for this
2. Fixing the default implementation of quo_rem

[jdemeyer]

comment:6

Replying to @nthiery:

I indered pondered about this. I went for using r.is_zero() for
style consistency with the nearby zero tests. But I don't have a
strong opinion, and I am fine switching to if r. In that case, maybe
the nearby zero tests should be changed accordingly for
consistency. What do you think?

I don't have a strong opinion. When choosing between different equivalent options like this, I would choose depending on efficiency and you cannot beat if r then. Feel free to change all zero tests like this.

[nthiery]

comment:7

FieldElement.quo_rem almost duplicates Fields.ElementMethods.quo_rem, with the same bug of returning a plain zero, but with the additional feature of doing coercion on rem.

Do you see any situation where this method could be time critical, or should I just remove FieldElement.quo_rem (and move up the coercion feature)?

[sagetrac-git]

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

8a3978c

19939: Improved EuclideanRings.ParentMethods._test_quo_rem to check the parents of the return values of quo_rem

[sagetrac-git]

Changed commit from 2d11e11 to 8a3978c

[sagetrac-git]

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

4a9c931

19939: fix doctests of Fields.ElementMethods.quo_rem to actually exercise its code

[sagetrac-git]

Changed commit from 8a3978c to 4a9c931

[sagetrac-git]

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

6e8e4c0

19939: fix the two duplicated implementations of quo_rem for Fields to be consistent and refer to each other

[sagetrac-git]

Changed commit from 4a9c931 to 6e8e4c0

[nthiery]

comment:11

Replying to @jdemeyer:

I don't have a strong opinion. When choosing between different equivalent options like this, I would choose depending on efficiency and you cannot beat if r then. Feel free to change all zero tests like this.

Thinking twice about it: this _test_method is going to be run just once for each parent, so speed is not an issue here. Besides is_zero is more explicit for the reader, especially for new developers who may stumble upon this while learning to implement an EuclideanRing.

Changing all is_zero consistently can be done in a later ticket, if someone cares.

Thanks for your feedback though!

[slel]

Changed branch from u/nthiery/fix_euclideanrings_parentmethods__test_quo_rem_to_use__not_r_is_zero____rather_than__r__0_ to u/slelievre/fix_euclideanrings_parentmethods__test_quo_rem_to_use__not_r_is_zero____rather_than__r__0_

[slel]

comment:13

Polishing. Jeroen, I'll let you give positive review if you're satisfied.

[slel]

Changed branch from u/slelievre/fix_euclideanrings_parentmethods__test_quo_rem_to_use__not_r_is_zero____rather_than__r__0_ to u/nthiery/fix_euclideanrings_parentmethods__test_quo_rem_to_use__not_r_is_zero____rather_than__r__0_

[slel]

Changed reviewer from Samuel Lelièvre to Jeroen Demeyer, Samuel Lelièvre

[nthiery]

Changed reviewer from Jeroen Demeyer, Samuel Lelièvre to Samuel Lelièvre

[nthiery]

New commits:

8a3978c	19939: Improved EuclideanRings.ParentMethods._test_quo_rem to check the parents of the return values of quo_rem
4a9c931	19939: fix doctests of Fields.ElementMethods.quo_rem to actually exercise its code
6e8e4c0	19939: fix the two duplicated implementations of quo_rem for Fields to be consistent and refer to each other

[nthiery]

comment:21

Hi Jeroen,

The patchbot is now ok. Could you confirm that you are ok with how your comments have been taken care of, and if yes set a positive review?

Thanks!

---

New commits:

e760680	Merge branch 'develop' into t/19939/fix_euclideanrings_parentmethods__test_quo_rem_to_use__not_r_is_zero____rather_than__r__0_
eda4420	19939: two trivial ReST fixes

[nthiery]

Changed commit from 269155d to eda4420

[jdemeyer]

comment:22

I don't like this:

if not have_same_parent(self, other):
    other = self.parent()(other)

You should use coercion instead.

[jdemeyer]

comment:23

But really, you don't even need coercion. You can just compute self/other and that will do the coercion for you.

[jdemeyer]

comment:24

The question is: what should be the parent of the zero remainder?

[jdemeyer]

comment:25

I think that the parent of the remainder should be equal to the parent of the quotient. So I propose something like

def quo_rem(self, other):
    quo = self / other
    return quo, quo.parent().zero()

[nthiery]

comment:26

+1!
Will try to do this change soon.

[jdemeyer]

comment:27

Maybe it's safer to do

def quo_rem(self, other):
    quo = self / other
    return quo, parent(quo)(0)

since we're not guaranteed that the quotient is a Sage Element.

[slel]

Changed reviewer from Samuel Lelièvre to Jeroen Demeyer, Samuel Lelièvre

[mkoeppe]

comment:30

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

[mkoeppe]

comment:31

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