Skip to content

Fixes #7228: Round °F temperature to one decimal place #7307

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

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Fixes #7228: Round °F temperature to one decimal place #7307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions netbox/project-static/dist/status.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion netbox/project-static/dist/status.js.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion netbox/project-static/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ export function createElement<
* @returns Degrees in Fahrenheit.
*/
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

@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.

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

@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.

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

@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.

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

@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.

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 :)

}

/**
Expand Down