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.

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

Issue with Round Function in Python #123811

Open
lykan89 opened this issue Sep 7, 2024 · 18 comments
Open

Issue with Round Function in Python #123811

lykan89 opened this issue Sep 7, 2024 · 18 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@lykan89
Copy link

lykan89 commented Sep 7, 2024

Bug report

Bug description:

Description of the Issue:
The round function does not seem to handle certain floating-point numbers correctly. For example, when rounding the number -1357.1357 to negative 4 decimal places, the function returns -0.0 instead of the expected 0.0.

Steps to Reproduce:

  1. Open a Python interpreter.
  2. Execute the following code:
print(round(-1357.1357, -4))
  1. Observe the output.

Expected Result: The output should be 0.0.
Actual Result: The output is -0.0.

Environment Details Tested:
--Python Version: 3.12.5, Operating System: Windows 11.
--Python Version: 3.12.3, Operating System: Ubuntu.

Additional Information: I have verified this issue on multiple machines and with different Python versions. The issue persists across all tested environments.

CPython versions tested on:

3.12, 3.13

Operating systems tested on:

Linux, Windows

Linked PRs

@lykan89 lykan89 added the type-bug An unexpected behavior, bug, or error label Sep 7, 2024
@ZeroIntensity
Copy link
Member

Hmm, is this causing any actual problems in practice? 0.0 and -0.0 are, more or less, equivalent numbers.

@serhiy-storchaka
Copy link
Member

This is an intentional behavior. It is here at least since e6a076d (which fixed bpo-1869, bpo-4707, bpo-5118, bpo-5473, bpo-1456775) and may be even earlier. It is consistent between float and Decimal.

Although this case is not explicitly documented and tested.

@serhiy-storchaka
Copy link
Member

@mdickinson, @tim-one, what are your thoughts? Should we explicitly document that round() can return a negative zero?

I think that we should add tests for this in any case, to prevent code degradation.

@tim-one
Copy link
Member

tim-one commented Sep 7, 2024

It shouldn't be changed. It's a form of "underflow to 0", and all relevant standards require that the sign bit be retained in such cases. Python's round() isn't mentioned in the standards, so this is a "to be consistent with the obvious intent of ..." thing.

Mixed feelings about documenting it. Hardly anyone will care, and filtering out irrelevant (to them) "doc bloat" is an ongoing burden for all other readers. If it has to be documented, I'd recommend a footnote.

But, yes, it absolutely should be tested.

@skirpichev
Copy link
Member

skirpichev commented Sep 8, 2024

In some sense, it's tested, e.g. here:

self.assertEqual(round(-123.456, n), -0.0)

This test just ignores difference between 0.0 and -0.0. Probably, that line could be just changed to use assertFloatsAreIdentical() (see also #121071). Or did I miss something and we need more test cases?

Edit: pr is ready, #123829

@tim-one
Copy link
Member

tim-one commented Sep 8, 2024

This isn't worth much effort. assertFloatsAreIdentical() certainly - equality testing is useless (+0.0 == -0.0). In addition to checking that round(-123.456, n) returns -0.0, also check that +0.0 is returned if the first argument is negated. Good enough, I think.

@skirpichev
Copy link
Member

also check that +0.0 is returned if the first argument is negated

Yeah, that one was right above. I just did replace for all tests with ±0.0 on rhs.

But I dislike adding assertFloatsAreIdentical() helper yet in another place. So, I would appreciate if something like #121071 will be merged first.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 11, 2024
…23829)

(cherry picked from commit d2b9b6f)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 11, 2024
…23829)

(cherry picked from commit d2b9b6f)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@vstinner
Copy link
Member

@skirpichev: Can we close this issue or do you plan further changes?

@skirpichev
Copy link
Member

@vstinner, on my taste - documentation shouldn't be changed. This looks as a very special corner case with natural behaviour for whose who do care about distinction between 0.0 and -0.0. Most users, probably, won't notice this because zeros are equal.

As Serhiy asked Mark too - lets wait his opinion. Tim Peters seems to be -0 on this.

vstinner added a commit that referenced this issue Sep 11, 2024
#123939)

gh-123811: test that round() can return signed zero (GH-123829)
(cherry picked from commit d2b9b6f)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
@tim-one
Copy link
Member

tim-one commented Sep 11, 2024

Mark is no longer a CPython core dev, so while I too would like his opinion, I don't expect him to respond. FWIW, I can't recall a case where we ever disagreed 😉. Yes, I'm -0 on spelling this out in the docs.

@skirpichev
Copy link
Member

Mark is no longer a CPython core dev

This looks wrong for me.

Yes, I'm -0 on spelling this out in the docs.

Ok, no fans of this idea so far. Probably, this can be closed.

@tim-one
Copy link
Member

tim-one commented Sep 12, 2024

@skirpichev, I don't think Mark was pressured to quit, On the way out. he emailed me saying he was resigning for several reasons, but only disclosed that "dismay" over my case was one of them. He registered his feelings by clicking on Ethan's dissenting reply to my ban announcement. If you agree, you could too 😉.

I sure miss Mark too! We were colleagues and pals here for about 15 years, He was easily the best of CPython's "number nerds", both technically and by 100% consistent good nature. He was a joy to work with.

But we don't really need his giant brain for this one - it's a minor corner case where the code is already working as intended, and no reason given to change it,. IMO negative zeroes were always a dubious idea, but the world is stuck with them now. Given that, we're doing the best that can be done with them. "Surprises" come with the territory.

@vstinner
Copy link
Member

Nobody wants to change the doc, so let met close the issue. Thanks @skirpichev for adding tests (fixing tests in fact).

@lykan89: What you get is the expected behavior. I know, floating point numbers are surprising :-)

@serhiy-storchaka
Copy link
Member

Is this tested for Decimal?

@skirpichev
Copy link
Member

Is this tested for Decimal?

Good catch. I see no tests or doctests. Will add.

@serhiy-storchaka
Copy link
Member

Fist try to make __round__() converting a negative zero to positive zero and check whether anything fails.

@skirpichev
Copy link
Member

New pr: #124007

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 13, 2024
…honGH-124007)

(cherry picked from commit b46c65e)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 13, 2024
…honGH-124007)

(cherry picked from commit b46c65e)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
serhiy-storchaka pushed a commit that referenced this issue Sep 13, 2024
…-124007) (GH-124049)

(cherry picked from commit b46c65e)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@skirpichev
Copy link
Member

Two backport pr aren't merged.

@AlexWaygood AlexWaygood reopened this Sep 17, 2024
Yhg1s pushed a commit that referenced this issue Sep 24, 2024
…-124007) (#124048)

gh-123811: Test that round(Decimal) can return signed zero (GH-124007)
(cherry picked from commit b46c65e)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

8 participants