Change Rational's round method default rounding from away to even

[vneiger]

📚 Description

Fixes #35473. Changes the default of the RationalNumber's round() from away to even. This follows up on #35474 : the commits in the latter are all contained here, and one additional commit is added to fix the testing error and improve the documentation.

📝 Checklist

• The title is concise, informative, and self-explanatory.
• 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 accordingly.

[mezzarobba]

• I agree with the idea of re-aligning this with Python, but it would be better to give users a transition period. How about just deprecating the current behavior for now?
• Do I understand correctly that the proposed change would make the default rounding mode for rational numbers inconsistent with other number field elements? Then I believe we should either keep the current default or (deprecate then) change both.

[vneiger]

Concerning your second point, I agree consistency would be much better. I will have a look at it to see if I detect any potential issue in doing this.

Concerning deprecation, any clue/examples as to how to handle it in this case? I have only seen "whole functions" being deprecated, but here the goal would be to indicate that only one default argument will change in the future, right?

[mezzarobba]

Concerning deprecation, any clue/examples as to how to handle it in this case? I have only seen "whole functions" being deprecated, but here the goal would be to indicate that only one default argument will change in the future, right?

I would do something like

def round(Rational self, mode=None):
    if mode is None:
        from sage.misc.superseded import deprecation
        deprecation(...)
        mode = "away"

[vneiger]

I have made an attempt with the solution using deprecation, and also stated more clearly in the documentation for number field elements that the rounding mode relies on the default one for rationals.

There are still some doctests that are impacted and should be fixed, essentially with a lot of copy-pasting of the deprecation warning. So, I would appreciate some feedback on the current changes before doing these fixes.

[mezzarobba]

There are still some doctests that are impacted and should be fixed, essentially with a lot of copy-pasting of the deprecation warning. So, I would appreciate some feedback on the current changes before doing these fixes.

The changes look good to me. However, I don't understand why there are doctests that need to contain the deprecation warning in their output. Wouldn't it be possible to specify an explicit rounding mode in the corresponding calls instead?

[vneiger]

The changes look good to me. However, I don't understand why there are doctests that need to contain the deprecation warning in their output. Wouldn't it be possible to specify an explicit rounding mode in the corresponding calls instead?

This is possible for some of the doctests, but not all (and in fact, not many of those that fail). What I mean is that the call to rationals' round is often not directly appearing in the doctest itself but in some underlying call. My take was that we should not force the rounding mode to the current (deprecated) default in all these calls.

Consider for example the following test, in schemes/elliptic_curves/mod_sym_num.pyx:

M = E.modular_symbol(implementation="num")

There is no round appearing explicitly in this test, but the deprecation warning will be raised.

[mezzarobba]

I see. Perhaps we could issue the warning only when there is indeed an ambiguity, or have some way (mode="auto"? mode="default"?) of specifying that we don't care about the rounding mode and ignore the warning in that case? Or maybe display the warning unconditionally when mode is None, but only for half-integers when mode == "default")? I'm not really convinced by what I am suggesting...

[vneiger]

I see. Perhaps we could issue the warning only when there is indeed an ambiguity

Good point, yes, there is no reason to raise the warning when round is called in cases where the behaviour has not changed. I have modified this.

For the failing doctests, I have specified the rounding mode (to "away" which is the default) in a place which seemed to be responsible for most cases of failures. Let's see if that fixes all of them.

[github-actions]

Documentation preview for this PR (built with commit 07a90a9; changes) is ready! 🎉

[vneiger]

For the failing doctests, I have specified the rounding mode (to "away" which is the default) in a place which seemed to be responsible for most cases of failures. Let's see if that fixes all of them.

Confirmed, tests all go fine now. In short: the problem was only that the deprecation warning was appearing here and there; now the warning is only issued in case of ties, and the few remaining warnings in doctests disappear thanks to a single change in a file (making the rounding mode explicit instead of the default).

[mezzarobba]

Thanks Vincent!

[seblabbe]

In the code of my package, I now have few deprecation warnings which I tried to fix today.

The deprecation is obtained by the following easy example:

sage: b = x(x=5/2)
sage: b
5/2
sage: type(b)
<class 'sage.symbolic.expression.Expression'>
sage: round(b)
<ipython-input-25-2087a40e3ddb>:1: DeprecationWarning: the default rounding for rationals, currently `away`,
will be changed to `even`. See https://github.com/sagemath/sage/issues/35473 for details.
3

Ok, but then how should one fix that warning?

sage: round(b, mode='away')
Traceback (most recent call last)
...
TypeError: round() got an unexpected keyword argument 'mode'
sage: b.round(mode='away')
Traceback (most recent call last)
...
TypeError: round() got an unexpected keyword argument 'mode'

Also notice that for most types, there is no argument mode, so it is not so safe to add this argument in code where different type of values can be used...

sage: c = RDF(5/2)
sage: round(c)
2
sage: c.round()
2
sage: c.round(mode='away')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [34], line 1
----> 1 c.round(mode='away')

TypeError: round() got an unexpected keyword argument 'mode'

[vneiger]

Ok, but then how should one fix that warning?

I have not encountered this situation before, so I'd be interested to hear about other's solutions/opinions. In any case you should be able to filter these warnings out with something like:

import warnings
warnings.filterwarnings("ignore", category=DeprecationWarning)

(https://docs.python.org/3/library/warnings.html)

Interestingly, b.round() does return 3 (since current default is "away") without a warning. But that might not be a good workaround for other reasons.