Skip to content

Conversation

@royreznik
Copy link
Contributor

Fixes: #7228

Rounding the °F temperature to the first decimal place

before:
image

after:
temp

@jeremystretch jeremystretch merged commit b86847c into netbox-community:develop Sep 20, 2021
@royreznik royreznik deleted the fix-7228 branch September 20, 2021 14:23
*/
export function cToF(celsius: number): number {
return celsius * (9 / 5) + 32;
return Math.round((celsius * (9 / 5) + 32 + Number.EPSILON) * 10) / 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually work? Certain numbers cannot be represented exactly using floating point (0.1 is one example); rounding doesn't change this.

I would expect the problem is at the point of rendering it back to decimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please provide an example for this?

I checked it with 0.1 and 0.3 and still got the expected value

Math.round((0.1 + Number.EPSILON) * 10) / 10; # 0.1
Math.round((0.3 + Number.EPSILON) * 10) / 10; # 0.3

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I think I can replicate via Javascript console in Chrome:

image

The problem is indeed fixed by your method, but actually it would be simpler to multiply by 9 and then divide by 5, rather than multiply by (9/5) which is an inexact floating point value; it appears that the error in (9/5) is magnified by multiplying by the degrees C value.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no guarantee that doing *9/5 will give the right answer though. I believe the correct solution is to convert the number to a string with the correct number of decimal places, using toFixed:

> (42 * 1.8)
< 75.60000000000001
> (42 * 1.8).toFixed(2)
< '75.60'
> (42 * 1.8).toFixed(1)
< '75.6'

Copy link
Contributor Author

@royreznik royreznik Sep 20, 2021

Choose a reason for hiding this comment

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

As you said, There's no guarantee that doing *9/5 will give the right answer though, i wont going to change this

using only toFixed can result a different inacurate like this:

(1.005).toFixed(2) # 1.00

Copy link
Contributor

@candlerb candlerb Sep 20, 2021

Choose a reason for hiding this comment

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

Yes you're right:

> (1.006).toFixed(2)
< '1.01'
> (1.005).toFixed(2)
< '1.00'
> (1.00501).toFixed(2)
< '1.01'

and yet:

> (1.05).toFixed(1)
< '1.1'

Floating point is broken, and Javascript especially so :-(

However I think toFixed is OK here. I just want never to see a number with spurious 13 places of decimals. I don't mind if it's off by 0.1 degrees F, which it almost never will be anyway.

Copy link
Contributor Author

@royreznik royreznik Sep 20, 2021

Choose a reason for hiding this comment

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

I see what you meant. Floating point is evil :-(

also toFixed will do this:

(1).toFixed(1) // "1.0 "
Math.round((1 + Number.EPSILON) * 10) / 10; //  1

this will add .0 for integers.

Anyway, I think my solution fix the bug we had and maybe different approch can be achived in other PR :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rounding required for °F temperature conversion in Napalm device status tab

3 participants