Opened 17 months ago

Last modified 6 months ago

#25827 needs_info enhancement

py3: some care for round

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.9
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 (36)

comment:1 Changed 17 months ago by chapoton

  • Branch set to u/chapoton/25827
  • Commit set to f79a61ba4139a17258bd8b60ebf544b68d0f1fe5
  • Status changed from new to needs_review

New commits:

f79a61bsome changes about round

comment:2 Changed 17 months ago by chapoton

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

comment:3 Changed 17 months ago by chapoton

  • Status changed from needs_review to needs_work

failing doctests

comment:4 Changed 17 months ago by chapoton

  • Cc jdemeyer tscrim added

comment:5 Changed 17 months ago by embray

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

comment:6 Changed 15 months ago by chapoton

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

comment:7 Changed 15 months ago by embray

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

comment:8 Changed 15 months ago by embray

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.
  1. 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 15 months ago by chapoton

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 14 months ago by git

  • Commit changed from f79a61ba4139a17258bd8b60ebf544b68d0f1fe5 to 27cfe85a104b79299e12e3303c5a8d3ca51aa41e

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

27cfe85py3: introducing __round__ methods as alias for existing .round

comment:11 Changed 14 months ago by chapoton

  • 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

Last edited 14 months ago by chapoton (previous) (diff)

comment:12 Changed 14 months ago by embray

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?

comment:13 follow-up: Changed 14 months ago by embray

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.

comment:14 in reply to: ↑ 13 Changed 14 months ago by chapoton

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 14 months ago by embray

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) 
    30403040        """
    30413041        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 arbitrary-precision `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 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.

Last edited 14 months ago by embray (previous) (diff)

comment:16 Changed 14 months ago by embray

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...

comment:17 Changed 14 months ago by chapoton

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

comment:18 Changed 13 months ago by chapoton

  • Milestone changed from sage-8.3 to sage-8.5

comment:19 Changed 12 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:20 Changed 12 months ago by chapoton

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

8c5187cpy3: adding __round__ to real_mpfr.pyx

comment:21 Changed 11 months ago by chapoton

  • Milestone changed from sage-8.5 to sage-8.7

comment:22 Changed 11 months ago by chapoton

bot is morally green..

comment:23 follow-up: Changed 11 months ago by embray

  • 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 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.

comment:24 Changed 11 months ago by embray

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 11 months ago by git

  • Commit changed from 8c5187c0512b1be9d4a6a54732b34a9abb389d5e to 53280c597c105e6874bf0f8e28acbb6b9bf4563f

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

25e23f3Merge branch 'public/ticket/25827' in 8.6.b0
53280c5adding more doctests to __round__

comment:26 Changed 11 months ago by chapoton

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 11 months ago by embray

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.

comment:28 in reply to: ↑ 23 ; follow-up: Changed 10 months ago by 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 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.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 10 months ago by embray

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 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.

comment:30 in reply to: ↑ 29 Changed 10 months ago by mmezzarobba

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.

comment:31 follow-up: Changed 10 months ago by 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?)

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

Last edited 10 months ago by embray (previous) (diff)

comment:32 in reply to: ↑ 31 Changed 10 months ago by mmezzarobba

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.

comment:33 Changed 9 months ago by chapoton

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.

comment:34 Changed 9 months ago by embray

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

comment:35 Changed 9 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.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 6 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.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).

Note: See TracTickets for help on using tickets.