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

feat: add round function #53

Merged
merged 13 commits into from
Jun 30, 2024
Merged

feat: add round function #53

merged 13 commits into from
Jun 30, 2024

Conversation

shan-shaji
Copy link
Contributor

@shan-shaji shan-shaji commented Jun 29, 2024

Tip

The owner of this PR can publish a preview release by commenting /publish in this PR. Afterwards, anyone can try it out by running pnpm add radashi@pr<PR_NUMBER>.

Summary

The round function is designed to round a given number to a specified precision.

Related issue, if any:

sodiray/radash#61

What kind of change does this PR introduce?

Feature

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

No

@shan-shaji shan-shaji marked this pull request as ready for review June 29, 2024 17:47
@shan-shaji shan-shaji changed the title feat: Add round function feat: Add round function Jun 29, 2024
@aleclarson aleclarson self-requested a review as a code owner June 30, 2024 03:22
@shan-shaji shan-shaji changed the title feat: Add round function feat: add round function Jun 30, 2024
src/number/round.ts Outdated Show resolved Hide resolved
src/number/tests/round.test.ts Outdated Show resolved Hide resolved
Comment on lines 18 to 23
precision =
precision == null
? 0
: precision >= 0
? Math.min(precision, 307)
: Math.max(precision, -323)
Copy link
Member

@aleclarson aleclarson Jun 30, 2024

Choose a reason for hiding this comment

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

How can we verify these precision limits to be correct?

Interestingly, Lodash does this:

precision = precision == null 
  ? 0 
  : (precision >= 0 
    ? Math.min(precision, 292) 
    : Math.max(precision, -292))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aleclarson I added that check inspired from them.
Screenshot 2024-06-30 at 10 34 34 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also i found an issue related with this: lodash/lodash#4232

Copy link
Member

Choose a reason for hiding this comment

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

Here's what Claude Sonnet 3.5 says on the matter:

You're right to question this. Lodash's choice of 292 is indeed intentional and has a specific reason. Let me explain:

  1. Precision: While JavaScript can theoretically represent numbers up to 308 decimal places, it doesn't maintain full precision for all of those digits.

  2. Reliable accuracy: Lodash uses 292 as a more conservative limit to ensure reliable and consistent results across different JavaScript engines and environments.

  3. IEEE 754 standard: This limit is related to the IEEE 754 double-precision floating-point format, which guarantees about 15-17 decimal digits of precision.

  4. Safety margin: 292 provides a safety margin, avoiding potential rounding errors or inconsistencies that might occur when pushing to the absolute limits of JavaScript's number representation.

So, while 308 is technically the maximum exponent, Lodash's choice of 292 is a pragmatic one, prioritizing consistent and reliable results over the theoretical maximum. For most practical applications, this limit is more than sufficient and helps prevent subtle bugs that could arise from pushing the boundaries of floating-point precision.

I will push an update, along with the Lodash test suite and a reimplementation to support the edge cases that lodash.round does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @aleclarson for the explanation. So they chose 292 to ensure results are reliable and consistent across different JavaScript engines (e.g., V8 in Chrome, SpiderMonkey in Firefox) and environments (e.g., browsers, Node.js).

Copy link
Member

@aleclarson aleclarson Jun 30, 2024

Choose a reason for hiding this comment

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

I tracked down the original commit:
lodash/lodash@68ae6fc

Not much justification is given. If we can use technically “more correct” limits, we should as long as NaN is avoided. If someone reports misbehavior in some JS engine, we'll consider a breaking change then, but considering such large precision values are probably very rare in Javascript code, that won't happen anytime soon. Besides, the caller can always wrap/fork Radashi round to clamp the precision to 292 if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So to be on the safe side shall we stick with 292 then?

Copy link
Member

Choose a reason for hiding this comment

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

After some testing, I found that 292 is the upper limit that prevents NaN in all cases, while -323 works fine as a lower limit in all cases.

@aleclarson aleclarson merged commit eadf5d0 into radashi-org:main Jun 30, 2024
3 of 4 checks passed
@aleclarson
Copy link
Member

Thanks for the contribution! Hope to see more from you in the future.

Copy link

github-actions bot commented Jul 1, 2024

A new beta version 12.2.0-beta.af936c4 has been published to NPM. 🚀

To install:

pnpm add radashi@12.2.0-beta.af936c4

The radashi@beta tag also includes this PR.

See the changes

@aleclarson aleclarson added the new feature This PR adds a new function or extends an existing one label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This PR adds a new function or extends an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants