Methods __round__, __trunc__, __floor__, __ceil__

[fchapoton]

https://docs.python.org/3/reference/datamodel.html?highlight=__round__#object.__round__

We should define these special methods so that the built-in round and math.trunc, math.floor, math.ceil can operate on Sage numbers.

This can help to eliminate imports of floor and ceil from sage.functions throughout the Sage library, which pulls in all of sage.symbolic.

Related: In the global namespace, we have:

• round = sage.misc.functional.round,
• floor = sage.functions.other.floor,
• ceil = sage.functions.other.ceil,
• trunc - undefined

CC: @jdemeyer @tscrim

Component: python3

Author: Frédéric Chapoton

Branch/Commit: public/ticket/25827 @ 53280c5

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

[fchapoton]

New commits:

f79a61b

some changes about round

[fchapoton]

Commit: f79a61b

[fchapoton]

Branch: u/chapoton/25827

[fchapoton]

comment:2

Not sure what to do. This is not curing the disease. It seems that python3 builtin "round" is looking for __round__, unlike python2.

[fchapoton]

comment:3

failing doctests

[embray]

comment:5

I've implemented a few __round__ in my python3 branch. I'll see about making a ticket for those.

[fchapoton]

comment:6

@embray, where is your latest python3 branch ? could you please provide a branch for here, with the implemented __round__ ?

[embray]

comment:7

Ok, I'll try to get back to you about that soon.

[embray]

comment:8

I think we need to think about exactly how to implement Sage's round() built-in as well. Currently, some types in Sage have a .round() method which takes no arguments, and only rounds to an integer--in particular it always rounds up, it seems.

I find this a bit odd, but maybe there's a good reason. I wonder, because I find it surprising that RealNumber.round() does not take into account the MPFR rounding mode of the parent field. Why is that? Is it just for consistency's sake--that all real and floating-point numbers round up the same way?

I implemented a RealNumber.__round__() which takes into account the parent field's rounding mode, and works quite nicely, though it's incompatible with Sage's round() built-in which just calls RealNumber.round() for integer results. So when rounding to the nearest int you get one rounding behavior, but in other cases you get a different rounding behavior.

So two questions:

1. Is there any reason to have a RealNumber.__round__() that respects the parent field's rounding mode? It seems to make sense, but maybe it contradicts other assumptions in Sage that I'm not aware of.

2. Should we add an argument to .round() methods and make them equivalent to .__round__()? Or do we add a separate .__round__(), and make .round() just a special case equivalent to calling __round__() with no arguments?

[fchapoton]

comment:9

So far in sage, almost no round method takes a second argument:

git grep " round(self" src/sage
src/sage/matrix/matrix_double_dense.pyx:    def round(self, ndigits=0):
src/sage/rings/complex_arb.pyx:    def round(self):
src/sage/rings/number_field/number_field_element.pyx:    def round(self):
src/sage/rings/number_field/number_field_element_quadratic.pyx:    def round(self):
src/sage/rings/qqbar.py:    def round(self):
src/sage/rings/real_arb.pyx:    def round(self):
src/sage/rings/real_double.pyx:    def round(self):
src/sage/rings/real_mpfi.pyx:    def round(self):
src/sage/rings/real_mpfr.pyx:    def round(self):
src/sage/symbolic/expression.pyx:    def round(self):

[sagetrac-git]

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

27cfe85

py3: introducing `__round__` methods as alias for existing .round

[sagetrac-git]

Changed commit from f79a61b to 27cfe85

[fchapoton]

comment:11

ok, here is a new branch, just adding some aliases __round__ redirecting to round. I propose to simply do that for the moment, in order to advance the move towards python3.

EDIT: still missing would be __round__ for Integer class

[embray]

comment:12

I would really rather think about this more carefully, and perhaps rework how Sage's built-in round() function works (or maybe even get rid of it entirely, at least on Python 3, if the new __round__ special makes it obsolete. Not clear. And thoughts on my last comment?

[embray]

comment:13

So far in sage, almost no round method takes a second argument:

That's because it's really meant to mean round-to-the-nearest-integer, which is simpler than what Python's built-in round does, which can return a float truncated to N digits, or an integer. Just making __round__ point to some of these class's old round methods is broken.

There's also now __trunc__, __floor__, and __ceil__ and I'm not sure if we want to implement them or not: https://docs.python.org/3.6/reference/datamodel.html#object.round but perhaps that can be left as a separate issue.

[fchapoton]

comment:14

There's also now __trunc__, __floor__, and __ceil__ and I'm not sure if we want to implement them or not: https://docs.python.org/3.6/reference/datamodel.html#object.round but perhaps that can be left as a separate issue.

Those do not appear in the python3 log.

[embray]

comment:15

FWIW here's my implementation for MPFR reals:

diff --git a/src/sage/rings/real_mpfr.pyx b/src/sage/rings/real_mpfr.pyx
index 9b90c88..a82c4eb 100644
--- a/src/sage/rings/real_mpfr.pyx
+++ b/src/sage/rings/real_mpfr.pyx
@@ -3040,6 +3040,38 @@ cdef class RealNumber(sage.structure.element.RingElement)
         """
         return mpfr_get_d(self.value, (<RealField_class>self._parent).rnd)
+    def __round__(self, ndigits=0):
+        """
+        Implement support for Python 3's `round` builtin.
+
+        This is mostly equivalent to simply calling ``round(float(x),
+        ndigits)`` where ``x`` is a `RealNumber`.  The difference is that it is
+        still returns an arbitrary-precision `RealNumber` of precision
+        equivalent to ``self`` (or an `Integer` if ``ndigits=0``).  It also
+        uses the rounding mode specified on the parent field.
+
+        EXAMPLES::
+
+            TODO
+        """
+        cdef Integer z
+        cdef RealNumber r
+        cdef char* s
+
+        if ndigits < 0:
+            return (<RealField_class>self._parent).zero()
+        elif ndigits == 0:
+            z = PY_NEW(Integer)
+            mpfr_get_z(z.value, self.value, (<RealField_class>self._parent).rnd
+            return z
+        else:
+            rnd = (<RealField_class>self._parent).rnd
+            mpfr_asprintf(&s, "%.*R*f", <int>ndigits, rnd, self.value)
+            r = self._new()
+            mpfr_set_str(r.value, s, 10, rnd)
+            mpfr_free_str(s)
+            return r
+

But I'm not 100% sure if it makes sense to do things this way or not (and it needs examples). However, it's broken with Sage's round() global built-in, because that tries to call a class's .round() method first if it exists. And in fact RealNumber does have an existing .round() method that's different and doesn't take into account the field's rounding mode as my above implementation does.

If anything .round() should be the same as .__round__(n=0); that or .round() grows an optional extra argument.

[embray]

comment:16

Perhaps it is a question that needs to be brought up on sage-devel, if we don't know the answers.

For example, why does Sage have its own round() built-in in the first place (at least part of the answer to that question seems to be exactly that something like __round__ did not exist in the first place)? Do we still need it, or at least, do we need it with Python 3? Do we like the semantics of Python's round or would we rather replace it with something else? To what extent do we want to support those semantics of different class's .round() methods? Etc...

[fchapoton]

comment:17

I have made #26412 for the same thing in integer.pyx

[mkoeppe]

comment:38

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

[mkoeppe]

comment:40

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:41

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