-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix(range): round value to same number of decimal places as props to avoid floating point rounding errors #27375
Merged
Merged
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ab99bb0
add test
a2420ee
lint
ae1f2ea
round to max num of decimal places between min, max, and step
3537810
abstract logic into util
4190e61
Merge remote-tracking branch 'origin/main' into FW-4132
47d0711
rename util
e48cf1c
fix wrong function name in test
4c61735
add comments about precision limitations
d295ee4
lint
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { getDecimalPlaces, roundToMaxDecimalPlaces } from './index'; | ||
|
||
describe('floating point utils', () => { | ||
describe('getDecimalPlaces', () => { | ||
it('should calculate decimal places for a float', async () => { | ||
const n = getDecimalPlaces(5.001); | ||
expect(n).toBe(3); | ||
}); | ||
|
||
it('should return no decimal places for a whole number', async () => { | ||
const n = getDecimalPlaces(5); | ||
expect(n).toBe(0); | ||
}); | ||
}); | ||
|
||
describe('roundToMaxDecimalPlaces', () => { | ||
it('should round to the highest number of places as references', async () => { | ||
const n = roundToMaxDecimalPlaces(5.12345, 1.12, 2.123); | ||
expect(n).toBe(5.123); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
export function getDecimalPlaces(n: number) { | ||
if (n % 1 === 0) return 0; | ||
return n.toString().split('.')[1].length; | ||
} | ||
|
||
/** | ||
* Fixes floating point rounding errors in a result by rounding | ||
* to the same specificity, or number of decimal places (*not* | ||
* significant figures) as provided reference numbers. If multiple | ||
* references are provided, the highest number of decimal places | ||
* between them will be used. | ||
* | ||
* The main use case is when numbers x and y are added to produce n, | ||
* but x and y are floats, so n may have rounding errors (such as | ||
* 3.1000000004 instead of 3.1). As long as only addition/subtraction | ||
* occurs between x and y, the specificity of the result will never | ||
* increase, so x and y should be passed in as the references. | ||
* | ||
* If multiplication, division, or other operations were used to | ||
* calculate n, the rounded result may have less specificity than | ||
* desired. For example, 1 / 3 = 0.33333(...), but | ||
* roundToMaxDecimalPlaces((1 / 3), 1, 3) will return 0, since both | ||
* 1 and 3 are whole numbers. | ||
* | ||
* @param n The number to round. | ||
* @param references Number(s) used to calculate n, or that should otherwise | ||
* be used as a reference for the desired specificity. | ||
*/ | ||
export function roundToMaxDecimalPlaces(n: number, ...references: number[]) { | ||
const maxPlaces = Math.max(...references.map((r) => getDecimalPlaces(r))); | ||
return Number(n.toFixed(maxPlaces)); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why did you opt for this approach instead of multiplying by a scaled
step
(and then dividing by the scale amount)?For example:
Scaling the value ensures that you deal with integers to avoid JavaScript's floating point precision funkiness. As long as you scale down by the same amount you scaled up, you should get a nice neat decimal as a multiple of
step
.The benefit of the above approach is that you can round and then clamp, whereas the proposed changes would do round, clamp, and then round again.
edit: Though I suppose if your step was
0.001
you'd be back to a decimal, but you could increaseSCALE
to a larger number like10000
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it wouldn't work for all numbers. I'd rather not set an arbitrary limit on how high or low the props can be, nor the precision they can have. For example, a
SCALE
of10000
sets the bounds within 0.0001 < n < (MAX_INT / 10000), and you can't have more than 4 decimal places -- 5.12345 * 10000 = 51234.5.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah good point. Though I think the current approach may have some accuracy issues too since
toFixed
can sometimes yield funky results.Video sample:
floating-point.mov
Is there a defined level of precision we support with
ion-range
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening there is you've got a
max
with the same number of decimal places as the result with the rounding errors, andtoFixed
is rounding to as many as themax
, so the error isn't trimmed. I would say that's okay, since we wouldn't be able to tell the difference between a rounding error result and something legitimate in a case like that. (While in your case, the only legitimate value with that precision is the actualmax
, if the step were 0.00000000000000001 instead, the two would definitely be indistinguishable.)I think it's okay that this means we implicitly only support up to a certain level of precision, because the problem is with floating point math itself, not Ionic. If people are using precision levels that high, they're going to run into rounding error issues like this all over the place. 🤭 (Or in other words, JS itself only supports up to a certain level of precision anyway, unless you start using third party data types meant for this sort of thing. And I'd say supporting those is out of scope for this fix 😆)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! Could we add a little bit of explanation about these limitations in the code so other devs are aware?