Fix incorrect tests and override canonical associate

[cxzhong]

Because one use roundtieeven, one use roundawayzero. They are not equal when half integers
And also move passagemath/passagemath#1770 into this file

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

⌛ Dependencies

[cxzhong]

I finally think this. If we need to preserve round to mpfr_round as the mpfr library semantic. So the tests actually not right.
@dimpase @nbruin

[github-actions]

Documentation preview for this PR (built with commit 746b07c; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[user202729]

Related to the rounding problem: #39486 and linked pull requests from there.

The rounding results are supposed to agree. That it doesn't is a bug, it's just nobody have gotten around to decide what to do with it yet. (Changing it is sort of a breaking change)

[cxzhong]

Related to the rounding problem: #39486 and linked pull requests from there.

The rounding results are supposed to agree. That it doesn't is a bug, it's just nobody have gotten around to decide what to do with it yet. (Changing it is sort of a breaking change)

OK, I know. I think maybe this file is a wrapper of mpfr library. and mpfr_round use the away method as the default method. Maybe It should be discuss

[nbruin]

The discussion from #41268 is relevant here: There it was proposed to bring RealField's rounding in line with TieToEven. The discussion there did not take into account that the failures are a knock-on effect of changing the default rounding on QQ, which then triggered the change in the rounding on quadratic integers. There it bumped into the rounding on RealField. I think this clearly illustrates that these are indeed breaking changes (if only for the tests in sagemath!)

Without extensive context, the sensible thing to do on RealField is "just do what MPFR does", since that's the main provider for functionality. MPFR decided to round away from zero on ties. I don't know why, but I think that's been compatible with most of sage until recently. It would be good to know if they have good reasons.

IEEE-754 is silent about what "round to nearest integer" should do; probably because for many floats it's a nonsensical operation. There are many applications where one just actively ensures that rounding does make sense. Those cases tend to stay away from ties, so the boundary behaviour doesn't really matter for actual applications.

IEEE-754 does have comments on how to round to the nearest representable float and does give a preference to TiesToEven. MPFR has a whole slew of rounding modes available. They are set in a "context". In RealField this is specified by the rnd mode. The default mode is RNDN, which is round-to-even (and the documentation quotes Knuth's preference for that mode).

In a variable precision implementation like RealField we can emulate round to nearest integer by a change of precision:

[EDIT: this is a sensible rounding method but it's NOT round to nearest integer. For instance, for RNDD it's basically floor, as @user202729 points out below]

def myround(x):
    s,m,e = x.sign_mantissa_exponent()
    if e > 0:
        return s*m*(2**e)
    p = x.prec()
    rnd = parent(x).rounding_mode()
    # the case p=e would need extra handling, since mpfr doesn't like 0 bit mantissas
    rounded = RealField(p+e,rnd=rnd)(x)
    sr, mr, er = rounded.sign_mantissa_exponent()
    if er == 0:
        return sr*mr
    elif er == 1:
        return sr*mr*2
    else:
        raise RuntimeError("this rounding should not happen")

This code is obviously not how one should actually implement this, but it illustrates the process. so I think an argument can be made that it's reasonable to use the rounding mode on RealField to round nearest integers as well.

Once again, I think this is a lot of work for something that basically doesn't matter for floats. But if people think it's really important to align float rounding with rounding on exact types like QQ and number fields then I think this would be the way to go.

Changing this would have ripple effects for RIF and possibly RealBallField (and then their complex counterparts as well) because those types are supposed to track provably what happens with intervals of real numbers (with the boundaries specified by mprf represented floats). Rounding may not be the most important operation there (and certainly not cases where the boundary comes into play!), but you do risk introducing inconsistencies in float-based code that IS trying to be rigorous. The code in those libraries has been informed by MPFR implementation choices, so you'd be opening a huge can of worms by making changes there, possibly opening yourselves up for having to make changes in all our uses for MPFI and ARB. Those are very delicate libraries. I don't think we have the expertise to replicate the careful consideration and analysis that went into their implementations and it would certainly be a very thankless job to do the work required (the articles on the algorithms have already been written, so the work would basically go without academic recognition).

So I think it's not so much the MPFR round itself that we need to respect but the whole MPFI and Arb beasts that are lurking behind it. A more careful analysis may show it's not so bad, but there is definitely cause for treading carefully.

[user202729]

Changing this would have ripple effects for RIF and possibly RealBallField (and then their complex counterparts as well) because those types are supposed to track provably what happens with intervals of real numbers (with the boundaries specified by mprf represented floats).

RIF uses mpfi, which uses mpfr.

RR uses mpfr.

RIF doesn't use RR. Changing default rounding mode in RR shouldn't affect RIF.

Edit: RIF does use RR.round(), but only in RIF.round() and RIF.round_unique().

I argue that the new behavior is "correct" (with respect to the new round):

• Let a be a RIF. Then if b = a.round(), the computation is correct if, for all x ∈ a, x.round() ∈ b.
• With the old behavior, for all x ∈ a, x.round_old() ∈ b.
• With the new behavior, for all x ∈ a, x.round_new() ∈ b.
• The value of the b interval change, but that's fine.

mpfr itself doesn't "have" a default rounding mode. The rounding mode is passed to each function call (like mpfr_add), and is stored in Sage's parent objects.

---

mpfr_round is a special case. Instead of having

mpfr_round (mpfr_t rop, mpfr_t op, mpfr_rnd_t rnd) 

they have

Function: int mpfr_rint (mpfr_t rop, mpfr_t op, mpfr_rnd_t rnd)
Function: int mpfr_round (mpfr_t rop, mpfr_t op)
Function: int mpfr_roundeven (mpfr_t rop, mpfr_t op)

Since they provide both, it's hard to say mpfr_round is the "default" and mpfr_roundeven or mpfr_rint is the "non-default". Most likely explanation is either

• mpfr_roundeven didn't exist when the code was first written, or
• the implementer overlook it, or
• the implementer didn't think about consistency with other parts.

---

A non-breaking way would be

• deprecate the function (which will deprecate round(RR(...)) as well)
• as a consequence, deprecate AA.round(), RIF.round(), RIF.round_unique() as well
• introduce RR.round_away_from_zero(), etc.
• fix the Sage library's components that use round(RR(...)) to use the new method instead
• introduce RR.round_even(), AA.round_even(), RIF.round_even(), RIF.round_unique_even() with a global setting such as default_round_to_even(True) which would redirect round() to round_even()

[nbruin]

From the documentation

https://www.mpfr.org/mpfr-current/mpfr.html#index-mpfr_005frint

I would say that mpfr_rint(x,parent(x).rounding_mode()) is the right rounding for us. Note that our use of rounding is not quite the floating point one: we return an actual integer.

I wasn't aware that mpfr had all these different rounding functions; basically on equal footing. Clearly they didn't make a choice for what the "default" should be. It actually looks like the original author of RealField just took the thing that happened to have the same name in mpfr.

Looking at the blame: this routine goes back a LONG time. The rounding was always away from zero. First , round and trunc returned a float, while floor and ceil returned Integers. This was soon changed to the current state.

Note that mpfr also has the mpft_rint_* functions that do "double rounding". I think that only comes into play if the target has lesser precision than the original. Since we're producing a GMP integer, that shouldn't come into play.

If we let "round" use the rounding of the parent through mpfr_rint, then the current behaviour agrees with RealField(prec,rnd="RNDA"). The default is RealField(prec,rnd="RNDN"), which makes round behave consistent with python floats.

---

What you call "nonbreaking" will be very disruptive: anyone who happens to use rounding will be affected, whereas the change in reality probably doesn't affect any meaningful uses of "round" (given that rounding a tie is a red flag for an unreliable result already). It's a matter of fact that float operations are not guaranteed to be bit-for-bit compatible between different versions of the software. Our doctests even acknowledge that. If this is felt to be important enough to change, then I think it is less disruptive to deal with this through as a "bit change in float behaviour" than to go through the deprecation song-and-dance. The pain of that will outweigh benefits for a very long time.

[user202729]

deal with this through as a "bit change in float behaviour"

so basically #41268 ?

---

side note, I also propose global setting to set to new version, which I think is not very disruptive.

[nbruin]

Yes, it would be largely #41268, with the refinement that, instead of hardcoding RNDN one would take the rounding mode from the parent.

The mechanism of a "global setting" is problematic because of scope and even -- possibly -- parallelism. In any case, I think that is still very disruptive, because now you'll require everyone who uses round to still change their code to set a global flag.

It's probably a good idea to give run this by sage-devel because we only have a limited number of voices represented here and it is, technically, a breaking change (particularly because the documented behaviour of round would change).

[user202729]

one would take the rounding mode from the parent

I think what you have in mind is

• if parent's rounding mode is RNDD, round halfway case toward -∞,
• if parent's rounding mode is RNDU, round halfway case toward +∞,
• if parent's rounding mode is RNDZ, round halfway case toward 0,
• if parent's rounding mode is RNDN, round halfway case toward even.

which is a reasonable choice. (1)

It's probably a good idea to give run this by sage-devel because we only have a limited number of voices represented here and it is, technically, a breaking change (particularly because the documented behaviour of round would change).

agree.

---

Unimportant nitpicking:

(1): this is not what mpfr_rint do.

mpfr_floor to the next lower or equal representable integer (like mpfr_rint with MPFR_RNDD);

in other words, mpfr_rint(..., MPFR_RNDD) is floor.

The mechanism of a "global setting" is problematic because of scope and even -- possibly -- parallelism.

There's threading.local (or, better, contextvars)

you'll require everyone who uses round to still change their code to set a global flag.

not necessarily. My proposal is to provide two options, either different method name (round/round_away_from_zero/round_even) or global flag. Then everyone who uses round should change the method name.

[nbruin]

Unimportant nitpicking:

(1): this is not what mpfr_rint do.

mpfr_floor to the next lower or equal representable integer (like mpfr_rint with MPFR_RNDD);

in other words, mpfr_rint(..., MPFR_RNDD) is floor.

Oops. Indeed. I think that's actually important. It shows that they were right in implementing all these different functions (of course! why would they have done so otherwise!). And I guess they left out "round ties to even" because mpfr_rint(...,MPFR_RNDN does that.

It's not clear to me that there is demand for two rounding routines. It makes the most sense to run a proposal by sage-devel on whether the semantics of round should be changed from TiesAway ot TiesToEven. If only our floats were base 3 ...

Renaming, ContextVars and threadlocal stuff just makes this all more complicated and it would be unavoidable for someone who needs rounding somewhere. I don't think that will be welcomed by anyone for something as trivial as deciding where a rounding tie goes. You can make that part of the question to sage-devel.