Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rounding rational numbers: default behaviour is not up-to-date with Python #35473

Closed
1 task done
vneiger opened this issue Apr 10, 2023 · 0 comments · Fixed by #35756
Closed
1 task done

Rounding rational numbers: default behaviour is not up-to-date with Python #35473

vneiger opened this issue Apr 10, 2023 · 0 comments · Fixed by #35756
Labels
t: enhancement

Comments

@vneiger
Copy link
Contributor

vneiger commented Apr 10, 2023

Is there an existing issue for this?

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.

Problem Description

The documentation for RationalField's round method says

        Return the nearest integer to ``self``, rounding away from 0 by
        default, for consistency with the builtin Python round.

Yet the builtin Python round currently (since Python3 I think) rounds to nearest, with halfway case rounding to even. The first example in the documentation is (9/2).round() which yields 5, and consequently in Sage round(9/2) yields 5 as well; whereas in Python the same round(9/2) yields 4.

Proposed Solution

A simple solution may be to change the default to even, which is the current Python convention and is already implemented in this method round for rational field.

Alternatives Considered

Or, seemingly less ideal: one could keep the current behaviour and modify the documentation to highlight that this diverges from Python's builtin behaviour.

Additional Information

No response
@vneiger vneiger changed the title Rounding rational numbers: default behaviour is not up-to-date with Python<title> Rounding rational numbers: default behaviour is not up-to-date with Python Apr 10, 2023
@ShriKaranHanda ShriKaranHanda mentioned this issue Apr 10, 2023
Closed
5 tasks
@vneiger vneiger mentioned this issue Jun 12, 2023
Merged
5 tasks
@vbraun vbraun closed this as completed Jul 4, 2023
@tobiasdiez tobiasdiez mentioned this issue Feb 11, 2024
Closed
2 tasks
vbraun pushed a commit to vbraun/sage that referenced this issue Feb 18, 2024
sagemathgh-37306: Fix random test failures in `number_field_element_q…
4e533d0 
…uadratic`

    
Fix sagemath#37296.

Add `warnings.filterwarnings` to ignore `DeprecationWarning` in the
failing doctest, since it is an unrelated issue and does not affect
correctness of the test.

To be more specific, the warning only appears when the rounding error
will change, i.e. on $n + \frac{1}{2}$ (I believe). Here are some runs:

```python
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
<ipython-input-12-179149e36f01>:1: DeprecationWarning: the default
rounding for rationals, currently `away`, will be changed to `even`.
See sagemath#35473 for details.
  a = QQ.random_element(Integer(3), Integer(3)); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
<ipython-input-14-179149e36f01>:1: DeprecationWarning: the default
rounding for rationals, currently `away`, will be changed to `even`.
See sagemath#35473 for details.
  a = QQ.random_element(Integer(3), Integer(3)); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
```
    
URL: sagemath#37306
Reported by: grhkm21
Reviewer(s): Sebastian A. Spindler
vbraun pushed a commit to vbraun/sage that referenced this issue Feb 19, 2024
sagemathgh-37306: Fix random test failures in `number_field_element_q…
a6d0f12 
…uadratic`

    
Fix sagemath#37296.

Add `warnings.filterwarnings` to ignore `DeprecationWarning` in the
failing doctest, since it is an unrelated issue and does not affect
correctness of the test.

To be more specific, the warning only appears when the rounding error
will change, i.e. on $n + \frac{1}{2}$ (I believe). Here are some runs:

```python
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
<ipython-input-12-179149e36f01>:1: DeprecationWarning: the default
rounding for rationals, currently `away`, will be changed to `even`.
See sagemath#35473 for details.
  a = QQ.random_element(Integer(3), Integer(3)); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
<ipython-input-14-179149e36f01>:1: DeprecationWarning: the default
rounding for rationals, currently `away`, will be changed to `even`.
See sagemath#35473 for details.
  a = QQ.random_element(Integer(3), Integer(3)); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
```
    
URL: sagemath#37306
Reported by: grhkm21
Reviewer(s): Sebastian A. Spindler
vbraun pushed a commit to vbraun/sage that referenced this issue Feb 22, 2024
sagemathgh-37306: Fix random test failures in `number_field_element_q…
c3e76e1 
…uadratic`

    
Fix sagemath#37296.

Add `warnings.filterwarnings` to ignore `DeprecationWarning` in the
failing doctest, since it is an unrelated issue and does not affect
correctness of the test.

To be more specific, the warning only appears when the rounding error
will change, i.e. on $n + \frac{1}{2}$ (I believe). Here are some runs:

```python
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
<ipython-input-12-179149e36f01>:1: DeprecationWarning: the default
rounding for rationals, currently `away`, will be changed to `even`.
See sagemath#35473 for details.
  a = QQ.random_element(Integer(3), Integer(3)); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
<ipython-input-14-179149e36f01>:1: DeprecationWarning: the default
rounding for rationals, currently `away`, will be changed to `even`.
See sagemath#35473 for details.
  a = QQ.random_element(Integer(3), Integer(3)); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
sage: a = QQ.random_element(3, 3); _ = round(a)
```
    
URL: sagemath#37306
Reported by: grhkm21
Reviewer(s): Sebastian A. Spindler
ludopulles added a commit to ludopulles/lattice-estimator that referenced this issue Sep 19, 2024
@ludopulles
Fix CI
cca27c0 
Related: sagemath/sage#35473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
t: enhancement
Projects
None yet
Milestone
No milestone
1 participant
@vneiger