Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use arbitrary precision rationals as the underlying representation of numbers #1182

Merged
merged 13 commits into from
Mar 17, 2023

Conversation

yannham
Copy link
Member

@yannham yannham commented Mar 13, 2023

Closes #1163.

This pull request introduces arbitrary precision rationals as the underlying number representation for Nickel, instead of the current 64-bits float. The motivation and the rationale of the specific design choices are detailed in #1163.

Choice of the arithmetic library

There are a few Rust crates for doing arbitrary-precision arithmetic:

  • rust-gmp is a direct binding to the well established GMP C library. It's however quite dry and low-level. Doesn't support a WASM build
  • rug is a high-level interface to GMP, MPFR and MPC. Because of its C binding, Rug doesn't support a WASM build.
  • ibig is pure Rust, but only support big integers.
  • malachite is suggested in one of the previous's crate documentation if you need WASM. It seems quite complete and reasonably performant, also it doesn't have the same order of magnitude of usages/downloads.
  • scientific could work as well, but looks small (2 stars, no issues, no PRs on the repo).

Among the crates that are both WASM-capable and handle arbitrary precision rationals, I chose to go with malachite which seemed to be the more solid. It's not out of the question to migrate to a different library anyway, if needed.

@github-actions github-actions bot temporarily deployed to pull request March 13, 2023 19:26 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 13:33 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 14:15 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 16:02 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 17:44 Inactive
First draft of implementing Nickel numbers using arbitrary precision
rationals instead of 64-bits float. The rationale is to provide exact
precision for common arithmetic operations (understand addition,
division, subtraction, raising to an integer power, modulo), as opposed
to floats, that can incur rounding error on sometimes common decimal
values.

Some operations do need rounding: raising to a non-integer power, as
well as representation as a string (to_string/serialization).
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 17:49 Inactive
@yannham yannham force-pushed the task/arbitrary-precision-numbers branch from 2383913 to 7513f5b Compare March 14, 2023 17:50
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 17:54 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 18:26 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 14, 2023 19:15 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 15, 2023 13:25 Inactive
@yannham yannham marked this pull request as ready for review March 15, 2023 13:36
@yannham yannham mentioned this pull request Mar 15, 2023
if n < &Number::ZERO {
return Err(EvalError::Other(
format!(
"generate: expected the 1st argument to be a positive number, got {n}"
Copy link
Contributor

Choose a reason for hiding this comment

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

For the trace primop, I used the builtin.trace as the output prefix. Maybe this here should use array.generate as well? We're generally assuming that the functions in the stdlib are addressed by their full path (without trickery like let-rebinding).

Copy link
Member Author

Choose a reason for hiding this comment

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

On one hand, this is not the stdlib function, this is the underlying primop. All other primops are doing the same right now (well, excepted for trace, apparently). It's not advised, but not out of the question, to use them somewhere else than in the stdlib (e.g. for performance reasons, maybe? Or will we just forbid them in user-code? I'm not sure). Anyway, the whole primop error reporting is a mess 😅 it's a very valid point but this code has just been moved around IIRC. I propose to do a pass on primop error reporting separately.

Co-authored-by: Oghenevwogaga Ebresafe <gaga.ebresafe@tweag.io>
@yannham yannham added the merge-queue merge on green CI label Mar 17, 2023
@github-actions github-actions bot temporarily deployed to pull request March 17, 2023 15:36 Inactive
@yannham yannham merged commit a0f7ffa into master Mar 17, 2023
@yannham yannham deleted the task/arbitrary-precision-numbers branch March 17, 2023 17:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merge-queue merge on green CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use arbitrary precision numbers instead of floats as the default representation
3 participants