Documentation improvements for rounding methods

[marizee]

📚 Description

Fixes a typo in the documentation of `real_arb.pyx`. Improves documentation for the `round` method for the rings `RR` and `RDF`, by specifying explicitly how the ambiguous cases are dealt with.

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

⌛ Dependencies

None

[marizee]

While doing these changes I observed that the tie-breaking convention in RR (inherited from mpfr_round) and that in RDF (inherited from python3's round) do not coincide: is this not an issue ?

[vneiger]

@tscrim @vneiger

[tscrim]

No, it is not an issue. There is no expectation that they should be the same. RDF uses the machine doubles (and thus might be CPU dependent, I forget offhand), whereas RR is part of an implementation that can do different types of rounding conventions.

Also, in your changes, it should be ``self`` to be code-formatted.

[vneiger]

This PR:

I have just checked the changes made. They seem correct to me (up to changing into ``self``).

• RR round is calling the library mpfr (function mpfr_round) and the documentation of this mpfr function is consistent with the one given in this PR (and as was already announced in the doc, the rounding convention of RR does not impact this specific method, which only offers rounding to an integer).
• RDF round seems to rely on python3's round, which is using the "rounding to even" mode (see https://docs.python.org/3/library/functions.html#round ), thus consistent with the documentation change in this PR.

Consistency between RR and RDF

Looking at the mpfr documentation, I see that making RR and RDF consistent on this rounding convention would be extremely easy: it would suffice to call mpfr_round_even instead of mpfr_round. Consistency of such methods could be expected by (or convenient for) the user: both are called "round" and act on very closely related classes of objects. And, since until this PR there was no explanation of what the rounding did in ambiguous cases, such a change would in theory not break any existing code. Yet, I am not familiar with how SageMath usually deals with such changes: maybe it is indeed safer to keep the non-consistent behaviours, in case some users assumed things not promised by the documentation (?).

[tscrim]

@vneiger In some ways, I am actually arguing the opposite: The user should not expect any consistency as the fundamental underlying data types are completely different. RR is part of a family of arbitrary-but-fixed precision numbers, whereas RDF is specifically based on machine doubles. Granted, this is a technical point.

As far as changing the rounding mode, while seeming somewhat innocuous, it feels like a very significant change to a core part to me. As such, it must be discussed on sage-devel before such a change is done (and I fail to see any benefit to possibly breaking sensitive code in the wild as per the above).

[vneiger]

@tscrim Ok, noted, thanks for your clarifications. I was placing myself as a user who ignores this technical difference, but I am realizing only now that making doc/coding choices under this assumption is a bad idea. So, ok for accepting the inconsistency, and then I agree that changing RR's round is an unnecessary task.

[tscrim]

I don’t think that is an unreasonable viewpoint to take, but anyone who cares enough to do that kind of comparison can almost certainly understand the difference between RR and RDF.

However, it wouldn’t be a bad thing to include an option to choose the type of rounding, but it might not be worth the hassle…

[vneiger]

Looks good to me. @tscrim are you able to launch the workflows (which I suppose are necessary before merging)?

[tscrim]

Looks good to me. @tscrim are you able to launch the workflows (which I suppose are necessary before merging)?

No, I don't. I am not sure who even has such permissions.

@mkoeppe @dimpase Who has the ability to do this? Can we give this to all members of the triage team?

[tscrim]

@marizee I have a few doc formatting things that need to be addressed before a positive review.

[mkoeppe]

I am not sure who even has such permissions.

@mkoeppe @dimpase Who has the ability to do this? Can we give this to all members of the triage team?

@tscrim We are using the default GH setting here, "Require approval for first-time contributors" (see  https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks)
Only "Maintainers" can approve workflow runs. I don't think this can be extended to other roles.

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: 99d059f

[tscrim]

@mkoeppe Okay, thanks.

[tscrim]

Thank you.

[fchapoton]

This has broken the linter, fix up in #35552