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

weather: Fix incorrect rounding for negative temperatures #2194

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

FintasticMan
Copy link
Member

Thanks @tokiwee for reporting this issue!
Closes #2185 (if GitHub un-deletes it).

@FintasticMan FintasticMan added bug Something isn't working weather Bugs and PRs related to Weather labels Dec 9, 2024
@FintasticMan FintasticMan added this to the 1.16.0 milestone Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

Build size and comparison to main:

Section Size Difference
text 372848B 32B
data 948B 0B
bss 22536B 0B

@mark9064
Copy link
Member

Would it be worth extracting out rounding integer division to some shared code? It's somewhat annoying that this isn't part of the C++ stdlib really :(

Maybe an implementation similar to this? https://github.com/lucianpls/Rounding-Integer-Division
I haven't tried it though

@FintasticMan
Copy link
Member Author

FintasticMan commented Dec 10, 2024

What do you think of this? I've made a RoundedDiv function in the Utility namespace, that uses C++20 concepts so that we can have keep the code type-agnostic, while making sure that it only works for expected values.

Didn't you also use a rounded division somewhere in the AoD code? Would this solution also work there?

@mark9064
Copy link
Member

Wow that looks great. I didn't know that concepts allow this, I should read up on it. Yes I think this would work for AOD, I guess that would go in a separate PR though?

I want to double check correctness properly before approving, but otherwise LGTM

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't work as C++ demotes the dividend from signed to unsigned when the divisor is unsigned:

Suppose the following

   int a;
   // -1 / 5, rounded
   a = (-1 + (-1 >= 0 ? 5 : -5) / 2) / 5;
   // a = 0 as expected
   a = (-1 + (-1 >= 0 ? 5U : -5U) / 2) / 5U;
   // uh oh, a = 429496728

   // more simply
   a = -1 / 2u;
   // a = 2147483647

I didn't even know this was a thing??

@FintasticMan
Copy link
Member Author

Oh yeah of course. Integer promotion rules are really stupid. This conversion to unsigned also only happens when the values are ints or smaller, because that makes sense. Let me think of a good way to solve this then...

@mark9064
Copy link
Member

because that makes sense

of course... 😅 I love the C++ spec too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working weather Bugs and PRs related to Weather
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Celsius tempatures are wrong in the weather service
2 participants