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

Rounding-Error: Use proper banker's rounding #153

Open
orgua opened this issue Jul 6, 2023 · 4 comments
Open

Rounding-Error: Use proper banker's rounding #153

orgua opened this issue Jul 6, 2023 · 4 comments
Assignees
Labels
bug Something isn't working resolved-on-develop A changeset fixing this issue has been commiutted to the development branch

Comments

@orgua
Copy link

orgua commented Jul 6, 2023

Isn't that comparison (first line) wrong considering the following comment?

    if ((!(remainder < 0.5) || (remainder > 0.5)) && (number_.integral & 1)) {
      // exactly 0.5 and ODD, then round up
      // 1.5 -> 2, but 2.5 -> 2
      ++number_.integral;
    }

Link

I think it should be

    if (!((remainder < 0.5) || (remainder > 0.5)) && (number_.integral & 1)) {
@eyalroz
Copy link
Owner

eyalroz commented Jul 9, 2023

So, this is code from Marco Paland's original repository, which I haven't touched. It does seem a bit weird and I need to take another look.

... but I don't quite understand your suggestion, e.g. why do you suggest we avoid the equality operator.

@orgua
Copy link
Author

orgua commented Jul 9, 2023

yeah, this is inherited from the original repo, but it's still wrong compared to the embedded comment. it came up during linting of a code-base.

Doing it like that with an extra step might fix some weird float-behavior with very small remainders in the fraction-part?!

@eyalroz
Copy link
Owner

eyalroz commented Jul 9, 2023

So, I just made it Banker's rounding.

@eyalroz eyalroz closed this as completed Jul 9, 2023
@eyalroz eyalroz reopened this Jul 9, 2023
@eyalroz eyalroz self-assigned this Jul 9, 2023
@eyalroz eyalroz added the resolved-on-develop A changeset fixing this issue has been commiutted to the development branch label Jul 9, 2023
@eyalroz eyalroz changed the title Rounding-Error Rounding-Error: Use proper banker's rounding Nov 10, 2023
@HenryLin2000
Copy link

In fact, I found that when executing this block, there is always remainder<=0.5, because the remainder here is updated (in line 605)

printf/src/printf/printf.c

Lines 604 to 611 in f8ed5a9

if (precision == 0U) {
remainder = abs_number - (double) number_.integral;
if ((!(remainder < 0.5) || (remainder > 0.5)) && (number_.integral & 1)) {
// exactly 0.5 and ODD, then round up
// 1.5 -> 2, but 2.5 -> 2
++number_.integral;
}
}

The abs_number here is the absolute value of the original number, and the value of integral depends on whether the number is carried.
There are 3 situations at this time

  1. If the previous remainder>0.5 (carry), then the current remainder is a negative number
  2. If the previous remainder<0.5 (no carry), then the current remainder is between the interval [0,0.5)
  3. If the previous remainder=0.5 (special case), then the current remainder is also 0.5, and rounding processing is executed.

So, there is never remainder>0.5, and the condition here is really a bit strange.

@eyalroz eyalroz removed the resolved-on-develop A changeset fixing this issue has been commiutted to the development branch label Jul 19, 2024
eyalroz added a commit that referenced this issue Jul 24, 2024
…'s rounding in `get_components()`

* Also added a comment-instead-of-a-testcase for a command in which that roll-over is meaningful
@eyalroz eyalroz added resolved-on-develop A changeset fixing this issue has been commiutted to the development branch bug Something isn't working labels Jul 24, 2024
eyalroz added a commit that referenced this issue Jul 25, 2024
…'s rounding in `get_components()`

* Also added a comment-instead-of-a-testcase for a command in which that roll-over is meaningful
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved-on-develop A changeset fixing this issue has been commiutted to the development branch
Projects
None yet
Development

No branches or pull requests

3 participants