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

Adapt polyfill to current state of decimal proposal? #16

Open
jessealama opened this issue Jan 19, 2024 · 11 comments
Open

Adapt polyfill to current state of decimal proposal? #16

jessealama opened this issue Jan 19, 2024 · 11 comments

Comments

@jessealama
Copy link

Jesse here, one of the co-champions of the decimal proposal! As I am preparing to give an update to the TC39 plenary, I noticed your polyfill. This looks great! There was a time when the champions were considering an arbitrary-precision model for decimals, but after a lot of discussion we've settled on a fixed bit-width approach, IEEE 754 Decimal128 in particular. Would you be interested in updating your polyfill to reflect those changes? FWIW I have an npm library that implements Decimal128 (or, more precisely, the part of Decimal128 that's relevant to the proposal as it currently stands).

@Yaffle
Copy link
Owner

Yaffle commented Jan 19, 2024

Hello Jesse! It's great to hear from you.
Some thoughts:

I see that you have something like:
Decimal.add(x, y[, rounding])
Decimal.subtract(x, y[, rounding])
Decimal.multiply(x, y[, rounding])
Decimal.multiplyAndAdd (x, y[, rounding])
Decimal.divide(x, y[, rounding])
Decimal.remainder(x, y[, rounding])
// where only rounding.roundingMode will be used
Decimal.abs(x)
Decimal.cmp(x, y)
Decimal.prototype.isNegative
Decimal.prototype.isNaN
Decimal.prototype.coefficient
Decimal.prototype.exponent
Decimal.prototype.round(precision, roundingMode)
Decimal.prototype.toString()
Decimal.prototype.toFixed(precision[, roundingMode = "half-up"])
Decimal.prototype.toPrecision(precision[, roundingMode = "half-up"]);
Decimal.prototype.toExponential(fractionDigits[, roundingMode = "half-up"])
btw, spec misses some args for toExponential, and misses toPrecision completely.
Also, where is the unary negation? and why isNegative, not sign ?

I vote for something much shorter, like dec128.mul or d128.mul, otherwise it may look too bulky.
What are the performance requirements? well, if it is 110 bits, then perhaps, internal usage of the BigInt to store the significand is not bad. Because Bigint has some overhead, and double-double arithmetic could be faster in case 106 bits are needed. Well... may be there are some other smart ways to implement it.

@Yaffle
Copy link
Owner

Yaffle commented Jan 23, 2024

@jessealama
what should .coefficient/.exponent return when the value is +-0, +-Infinity, NaN ?

@Yaffle
Copy link
Owner

Yaffle commented Jan 23, 2024

@jessealama , why do you use Decimal.cmp not Decimal.compare if you have long names for everything else

@Yaffle
Copy link
Owner

Yaffle commented Jan 23, 2024

@jessealama , I have done some changes, but there are still a lot of work if to support multiplyAndAdd, reminder, and numbers without normalization

@Yaffle
Copy link
Owner

Yaffle commented Jan 26, 2024

@jessealama , non-normalized decimals looks pretty natural, well. i have not tested all cases

@Yaffle
Copy link
Owner

Yaffle commented Feb 1, 2024

@jessealama I have run the tests from https://github.com/jessealama/decimal128/tree/main/tests/dectest against my code and fixed few bugs, I see some tests fails when the result is rounded to infinity or not rounded, not sure if it is a bug of test suite or my code's bug

@jessealama
Copy link
Author

Awesome! Thanks for taking a look. Yes, supporting non-normalized values is a bit tricky. Is there anything you need some help with? I'd be happy to take a look at a PR, for instance.

@jessealama
Copy link
Author

@jessealama what should .coefficient/.exponent return when the value is +-0, +-Infinity, NaN ?

Those properties were intended to be private -- no need to worry about that!

@jessealama
Copy link
Author

There's just toString, no more toFixed or toExponential.

@munrocket
Copy link

I vote for something much shorter, like dec128.mul or d128.mul, otherwise it may look too bulky.

why do you use Decimal.cmp not Decimal.compare if you have long names for everything else

Absolutely agree with it, filled the same ideas here tc39/proposal-decimal#105
This is great that proposal become easy to polyfill!

@Yaffle
Copy link
Owner

Yaffle commented Mar 28, 2024

Awesome! Thanks for taking a look. Yes, supporting non-normalized values is a bit tricky. Is there anything you need some help with? I'd be happy to take a look at a PR, for instance.

not entirely true, support for non-normalized values ​​according to the ІЕЕЕ754 standard seems quite natural, I didn’t even have to change the code, the only place is exact division:
If value is represented as significand*10**exponent, where significand is integer :
For the sum: a+b the exponent to set for the result is min(a.exponent, b.exponent);
For multiplication - sum(a.exponent, b.exponent);
For division - diff(a.exponent, b.exponent)
When that value of exponent cannot be kept just keep as small as possible.
See examples at https://github.com/Yaffle/BigDecimal/blob/master/tests.js#L80

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

No branches or pull requests

3 participants