Skip to content

Fix floating-point arithmetic issues when props are decimals.#619

Open
jcfrancisco wants to merge 2 commits intoreact-component:masterfrom
DataDog:jcfrancisco/fix-floating-point-arith
Open

Fix floating-point arithmetic issues when props are decimals.#619
jcfrancisco wants to merge 2 commits intoreact-component:masterfrom
DataDog:jcfrancisco/fix-floating-point-arith

Conversation

@jcfrancisco
Copy link
Copy Markdown

Consider the following case:

      const value = 5.3;
      const props = {
        marks: {},
        step: 0.05,
        min: 0,
        max: 5.3
      };

I would expect the output of getClosestPoint to be 5.3. However, it returns 5.25.

This is because of floating point arithmetic issues with the function.

I've added a test case and taken a stab at figuring it out, making sure I continue to respect the Math.floor logic on maxSteps.

Comment thread src/utils.js
Comment on lines +24 to +31
export function getPrecision(step) {
const stepString = step.toString();
let precision = 0;
if (stepString.indexOf('.') >= 0) {
precision = stepString.length - stepString.indexOf('.') - 1;
}
return precision;
}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This function is untouched, moved it up because I'm using it elsewhere.

Comment thread src/utils.js Outdated
Comment on lines +73 to +98
parseFloat(closestPoint.toFixed(getPrecision(step)));
withPrecision(closestPoint, getPrecision(step));
Copy link
Copy Markdown
Author

@jcfrancisco jcfrancisco Jan 21, 2020

Choose a reason for hiding this comment

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

I can back out this refactor if it's unwanted

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 21, 2020

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.83%. Comparing base (5ba4181) to head (e4768ef).
⚠️ Report is 219 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
+ Coverage   95.40%   95.83%   +0.43%     
==========================================
  Files           2        2              
  Lines          87       96       +9     
  Branches       29       30       +1     
==========================================
+ Hits           83       92       +9     
  Misses          4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant