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

Use epsilon comparison instead of rounding #175

Conversation

emersion
Copy link
Collaborator

Use the formula abs(a - b) < epsilon to check whether two floating-point values are equal.

@emersion emersion requested a review from aiAdrian as a code owner July 11, 2024 13:02
@louisgreiner louisgreiner self-requested a review July 11, 2024 13:23
Copy link
Collaborator

@louisgreiner louisgreiner left a comment

Choose a reason for hiding this comment

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

Tested and approved, very cleaver 👍

@aiAdrian
Copy link
Collaborator

aiAdrian commented Jul 11, 2024

Looks good for the validation side, but does it also resolve the epsilon issue when the user enter any floating point number, for example:
he like to enter

128.4 = 2h 8min und 24s => console.log((128.4) % 60); => 8.400000000000006 which is not correct w.r.t. business 8.4 would be fine. but the 8.40000000000000 6 must be avoided.

@louisgreiner
Copy link
Collaborator

I does look good for validation and visualisation in netzgrafik and perlenkette components, even with weirder figures and changing the number of decimal displayed

@aiAdrian aiAdrian merged commit 28feaef into 168-bug-rounding-issues-with-decimal-numbers Jul 11, 2024
@aiAdrian aiAdrian deleted the emersion/168-bug-rounding-issues-with-decimal-numbers branch July 11, 2024 21:01
@emersion
Copy link
Collaborator Author

@aiAdrian, what do you mean exactly? Are you concerned about 8.400000000000006 getting saved into the store/database?

I personally think we should never care about rounding issues in internal code (intermediary values, computations, etc), and we should only care about them right at the end, when we actually display the value to the user (in that case, we want to round according to getTimeDisplayPrecision()) or compare it (in that case, compare with an epsilon with the formula in this PR).

Does that make sense?

@aiAdrian
Copy link
Collaborator

Yes, indeed i care about storing those values. Especially they make no sense. Under the assumption the stored value gets further exported to 3rd party it would good having at least in the database (JSON) "correct" data. This isn't related to the solved validation question. The addressed problem gets solved with #168

aiAdrian added a commit that referenced this pull request Jul 12, 2024
* problem fixed

* visual fix

* refactored and validator fixed

* small ui fix

* does this fix the caching issue?

* fix it now

* fix it now

* undo

* cache issue solved

* Wrong URL was used, replaced with public one

* undo change

* fix

* Use epsilon comparison instead of rounding (#175)

* fix

---------

Co-authored-by: u216993 <[email protected]>
Co-authored-by: Simon Ser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants