Opened 2 years ago
Last modified 4 days ago
#25827 needs_info enhancement
py3: some care for round
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  python3  Keywords:  
Cc:  jdemeyer, tscrim  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  public/ticket/25827 (Commits)  Commit:  53280c597c105e6874bf0f8e28acbb6b9bf4563f 
Dependencies:  Stopgaps: 
Description
Change History (39)
comment:1 Changed 2 years ago by
 Branch set to u/chapoton/25827
 Commit set to f79a61ba4139a17258bd8b60ebf544b68d0f1fe5
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
Not sure what to do. This is not curing the disease. It seems that python3 builtin "round" is looking for __round__
, unlike python2.
comment:4 Changed 2 years ago by
 Cc jdemeyer tscrim added
comment:5 Changed 2 years ago by
I've implemented a few __round__
in my python3 branch. I'll see about making a ticket for those.
comment:6 Changed 2 years ago by
@embray, where is your latest python3 branch ? could you please provide a branch for here, with the implemented __round__
?
comment:7 Changed 2 years ago by
Ok, I'll try to get back to you about that soon.
comment:8 Changed 2 years ago by
I think we need to think about exactly how to implement Sage's round()
builtin as well. Currently, some types in Sage have a .round()
method which takes no arguments, and only rounds to an integerin 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 sakethat all real and floatingpoint 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()
builtin 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?
comment:9 Changed 2 years ago by
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):
comment:10 Changed 2 years ago by
 Commit changed from f79a61ba4139a17258bd8b60ebf544b68d0f1fe5 to 27cfe85a104b79299e12e3303c5a8d3ca51aa41e
Branch pushed to git repo; I updated commit sha1. New commits:
27cfe85  py3: introducing __round__ methods as alias for existing .round

comment:11 Changed 2 years ago by
 Status changed from needs_work to needs_review
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
comment:12 Changed 2 years ago by
I would really rather think about this more carefully, and perhaps rework how Sage's builtin 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?
comment:13 followup: ↓ 14 Changed 2 years ago by
So far in sage, almost no round method takes a second argument:
That's because it's really meant to mean roundtothenearestinteger, which is simpler than what Python's builtin 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.
comment:14 in reply to: ↑ 13 Changed 2 years ago by
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.
comment:15 Changed 2 years ago by
FWIW here's my implementation for MPFR reals:

src/sage/rings/real_mpfr.pyx
diff git a/src/sage/rings/real_mpfr.pyx b/src/sage/rings/real_mpfr.pyx index 9b90c88..a82c4eb 100644
a b cdef class RealNumber(sage.structure.element.RingElement) 3040 3040 """ 3041 3041 return mpfr_get_d(self.value, (<RealField_class>self._parent).rnd) 3042 def __round__(self, ndigits=0): 3043 """ 3044 Implement support for Python 3's `round` builtin. 3045 3046 This is mostly equivalent to simply calling ``round(float(x), 3047 ndigits)`` where ``x`` is a `RealNumber`. The difference is that it is 3048 still returns an arbitraryprecision `RealNumber` of precision 3049 equivalent to ``self`` (or an `Integer` if ``ndigits=0``). It also 3050 uses the rounding mode specified on the parent field. 3051 3052 EXAMPLES:: 3053 3054 TODO 3055 """ 3056 cdef Integer z 3057 cdef RealNumber r 3058 cdef char* s 3059 3060 if ndigits < 0: 3061 return (<RealField_class>self._parent).zero() 3062 elif ndigits == 0: 3063 z = PY_NEW(Integer) 3064 mpfr_get_z(z.value, self.value, (<RealField_class>self._parent).rnd 3065 return z 3066 else: 3067 rnd = (<RealField_class>self._parent).rnd 3068 mpfr_asprintf(&s, "%.*R*f", <int>ndigits, rnd, self.value) 3069 r = self._new() 3070 mpfr_set_str(r.value, s, 10, rnd) 3071 mpfr_free_str(s) 3072 return r 3073
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 builtin, 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.
comment:16 Changed 2 years ago by
Perhaps it is a question that needs to be brought up on sagedevel, if we don't know the answers.
For example, why does Sage have its own round()
builtin 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...
comment:17 Changed 2 years ago by
I have made #26412 for the same thing in integer.pyx
comment:18 Changed 2 years ago by
 Milestone changed from sage8.3 to sage8.5
comment:19 Changed 23 months ago by
 Status changed from needs_review to needs_work
comment:20 Changed 23 months ago by
 Branch changed from u/chapoton/25827 to public/ticket/25827
 Commit changed from 27cfe85a104b79299e12e3303c5a8d3ca51aa41e to 8c5187c0512b1be9d4a6a54732b34a9abb389d5e
 Status changed from needs_work to needs_review
new tentative based on Erik's suggested patch
New commits:
8c5187c  py3: adding __round__ to real_mpfr.pyx

comment:21 Changed 22 months ago by
 Milestone changed from sage8.5 to sage8.7
comment:22 Changed 22 months ago by
bot is morally green..
comment:23 followup: ↓ 28 Changed 22 months ago by
 Status changed from needs_review to needs_info
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 builtin 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 builtin 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.
comment:24 Changed 22 months ago by
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.
comment:25 Changed 22 months ago by
 Commit changed from 8c5187c0512b1be9d4a6a54732b34a9abb389d5e to 53280c597c105e6874bf0f8e28acbb6b9bf4563f
comment:26 Changed 22 months ago by
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) ?
comment:27 Changed 22 months ago by
Try it and see. I believe it would worklook 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 wouldRealNumber.__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.
comment:28 in reply to: ↑ 23 ; followup: ↓ 29 Changed 21 months ago by
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 wouldRealNumber.__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.
comment:29 in reply to: ↑ 28 ; followup: ↓ 30 Changed 21 months ago by
Replying to mmezzarobba:
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 wouldRealNumber.__round__
not ignore it?My understanding is that
round()
,floor()
,ceil()
, andtrunc()
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 calledround()
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.
comment:30 in reply to: ↑ 29 Changed 21 months ago by
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 insage.functional
) round to a given precision. From my point of view, making them honor the parent's rounding mode when their argument already is aRealNumber
would be an improvement.  The
round()
methods ofRealNumber
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.
comment:31 followup: ↓ 32 Changed 21 months ago by
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?
comment:32 in reply to: ↑ 31 Changed 21 months ago by
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:
 IEEE754 specifies both a floatingpoint 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.
comment:33 Changed 20 months ago by
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 py3sage.
comment:34 Changed 20 months ago by
I think it would be good to bring it up, yet again, on the mailing list.
comment:35 Changed 19 months ago by
 Milestone changed from sage8.7 to sage8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:36 Changed 17 months ago by
 Milestone changed from sage8.8 to sage8.9
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).
comment:37 Changed 10 months ago by
 Milestone changed from sage8.9 to sage9.1
Ticket retargeted after milestone closed
comment:38 Changed 6 months ago by
 Milestone changed from sage9.1 to sage9.2
Moving tickets to milestone sage9.2 based on a review of last modification date, branch status, and severity.
comment:39 Changed 4 days ago by
 Milestone changed from sage9.2 to sage9.3
New commits:
some changes about round