Methods __round__, __trunc__, __floor__, __ceil__

fchapoton commented 5 years ago

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 commented 5 years ago

New commits:

f79a61b some changes about round

fchapoton commented 5 years ago

Commit: f79a61b

fchapoton commented 5 years ago

Branch: u/chapoton/25827

fchapoton commented 5 years ago

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 commented 5 years ago

comment:3

failing doctests

embray commented 5 years ago

comment:5

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

fchapoton commented 5 years ago

comment:6

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

embray commented 5 years ago

comment:7

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

embray commented 5 years ago

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:

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.
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 commented 5 years ago

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):

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from f79a61b to 27cfe85

fchapoton commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

comment:17

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

fchapoton commented 5 years ago

Changed commit from 27cfe85 to 8c5187c

fchapoton commented 5 years ago

Changed branch from u/chapoton/25827 to public/ticket/25827

fchapoton commented 5 years ago

comment:20

new tentative based on Erik's suggested patch

New commits:

8c5187c py3: adding `__round__` to real_mpfr.pyx

fchapoton commented 5 years ago

comment:22

bot is morally green..

embray commented 5 years ago

comment:23

I'm happy with the __round__ implementation of course, since I wrote it. But part of why I never submitted it in the first place is that I'm not totally confident with this course of action.

In particular, why does RealNumber.round ignore the rounding mode of the parent field? Is there some good reason for that? If so, then why would RealNumber.__round__ not ignore it?

There's also a problem that this __round__ is ignored anyways (as you can see by writing the tests to use the round() global instead of calling __round__() directly). The problem is that Sage's round() just ends up calling RealNumber.round() instead of RealNumber.__round__() and thus ignores the rounding mode, so this code effectively never gets used.

This is why I keep saying we maybe need to rethink the behavior of Sage's built-in round(). I'm starting to think that for Python 3 we might opt to just remove it entirely, or make it an alias for the Python built-in round(), since now we can override its behavior on different types by supplying __round__(), whereas previously we had to use this hack to override rounding on different types.

embray commented 5 years ago

comment:24

I also think that the test for this should demonstrate the fact that parent's rounding mode is considered. For example, demonstrate that with RNDN 2.5 is rounded to 2, while with RNDU it is rounded to 3.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 8c5187c to 53280c5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

`25e23f3`	`Merge branch 'public/ticket/25827' in 8.6.b0`
`53280c5`	`adding more doctests to __round__`

fchapoton commented 5 years ago

comment:26

Would it be ok to make the current round method an alias for the new __round__ method (just in this file for this kind of real numbers) ?

embray commented 5 years ago

comment:27

Try it and see. I believe it would work--look at the definition of round in sage.misc.functional.

It still doesn't answer my question though:

In particular, why does RealNumber.round ignore the rounding mode of the parent field? Is there some good reason for that? If so, then why would RealNumber.__round__ not ignore it?

I think that what you propose makes sense, but it would be a change in behavior (see the round(2.5) case) which I wouldn't want to make lightly unless nobody can provide a good explanation for the current behavior. This is what I keep trying to tell you. If neither of us know a reason for the current behavior then we need to find out who does know and ask them.

mezzarobba commented 5 years ago

comment:28

Replying to @embray:

In particular, why does RealNumber.round ignore the rounding mode of the parent field? Is there some good reason for that? If so, then why would RealNumber.__round__ not ignore it?

My understanding is that round(), floor(), ceil(), and trunc() are for rounding reals to integers, each in the way (≈ with the rounding mode) indicated by the name of the method. The parent's rounding mode only tells you how to round the results of inexact operations in that parent, and has no role to play here. That rounding to a certain precision can also be done with a function called round() is an accident, the two are unrelated.

embray commented 5 years ago

comment:29

Replying to @mezzarobba:

Replying to @embray:

In particular, why does RealNumber.round ignore the rounding mode of the parent field? Is there some good reason for that? If so, then why would RealNumber.__round__ not ignore it?

My understanding is that round(), floor(), ceil(), and trunc() are for rounding reals to integers, each in the way (≈ with the rounding mode) indicated by the name of the method. The parent's rounding mode only tells you how to round the results of inexact operations in that parent, and has no role to play here. That rounding to a certain precision can also be done with a function called round() is an accident, the two are unrelated.

Could you please be more specific? Are you talking about the Python stdlib or Sage? The fact is that the round() function in Python (and by extension in Sage) does allow rounding to N decimal digits.

I appreciate the effort to bring clarity to this question but to me this comment only muddies the waters.

mezzarobba commented 5 years ago

comment:30

Replying to @embray:

Could you please be more specific? Are you talking about the Python stdlib or Sage?

Both, I guess. What I'm trying to say (and I think it answers your question, but I haven't read the rest of the ticket, so perhaps I'm misunderstanding the issue) is that:

The round() functions (both the builtin and the one in sage.functional) round to a given precision. From my point of view, making them honor the parent's rounding mode when their argument already is a RealNumber would be an improvement.
The round() methods of RealNumber and several other element classes, however, are unrelated. They are for rounding to the nearest integer; and rounding up or down or whatever else is done with other methods.

embray commented 5 years ago

comment:31

Okay, thank you. That's a bit clearer. Though I don't quite understand "rounding up or down or whatever else is done with other methods". Given a RealNumber(2.5) should it round up or down (typically up I know, but why?)

And should the round() function ever call an element's .round() method?

mezzarobba commented 5 years ago

comment:32

Replying to @embray:

Okay, thank you. That's a bit clearer. Though I don't quite understand "rounding up or down or whatever else is done with other methods". Given a RealNumber(2.5) should it round up or down (typically up I know, but why?)

I missed that part of your question, sorry. Even when the rounding mode is “to nearest”, there are two (three?) competing conventions (up, or perhaps away from zero, in everyday life, vs to even). FWIW:

IEEE-754 specifies both a floating-point to integer conversion that rounds to nearest with halfway cases rounding to even and one with halfway cases rounding away from zero;
in C99/POSIX2001, round() rounds halfway cases away from zero regardless of the rounding mode. So I'd say it is best not take the rounding mode into account to decide what to do with ties, and we should round the absolute value up. But I have no strong opinion on the matter, really. That's just my understanding of the consensus.

And should the round() function ever call an element's .round() method?

As far as I understand, no.

fchapoton commented 5 years ago

comment:33

It would be good if somebody more knowledgeable than me would now take over this ticket and manage to push it into success. This problem with round is now one of the major source of doctest failures in py3-sage.

embray commented 5 years ago

comment:34

I think it would be good to bring it up, yet again, on the mailing list.

embray commented 5 years ago

comment:35

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

embray commented 5 years ago

comment:36

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

embray commented 4 years ago

comment:37

Ticket retargeted after milestone closed

mkoeppe commented 4 years ago

comment:38

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

mkoeppe commented 3 years ago

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 commented 2 years ago

comment:41

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

mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -1 +1,7 @@
+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`.
+
+

mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -4,4 +4,10 @@

 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.round`, 
+- `floor` = `sage.functions.other.floor`, 
+- `ceil` = `sage.functions.other.floor`,
+` `trunc` - undefined

+

mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -8,6 +8,6 @@
 - `round` = `sage.misc.round`, 
 - `floor` = `sage.functions.other.floor`, 
 - `ceil` = `sage.functions.other.floor`,
-` `trunc` - undefined
+- `trunc` - undefined

mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -5,7 +5,7 @@
 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.round`, 
+- `round` = `sage.misc.functional.round`, 
 - `floor` = `sage.functions.other.floor`, 
 - `ceil` = `sage.functions.other.floor`,
 - `trunc` - undefined

mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -7,7 +7,7 @@
 Related: In the global namespace, we have:
 - `round` = `sage.misc.functional.round`, 
 - `floor` = `sage.functions.other.floor`, 
-- `ceil` = `sage.functions.other.floor`,
+- `ceil` = `sage.functions.other.ceil`,
 - `trunc` - undefined

sagemath / sage

Methods round, trunc, floor, ceil #25827

sagemath / sage

Methods __round__, __trunc__, __floor__, __ceil__ #25827

Methods round, trunc, floor, ceil #25827