Yaffle / BigDecimal

a polyfill for decimal propocal
MIT License
51 stars 7 forks source link

Adapt polyfill to current state of decimal proposal? #16

Open jessealama opened 8 months ago

jessealama commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

Yaffle commented 8 months ago

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

Yaffle commented 8 months ago

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

Yaffle commented 8 months ago

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

Yaffle commented 8 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

munrocket commented 6 months ago

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 https://github.com/tc39/proposal-decimal/issues/105 This is great that proposal become easy to polyfill!

Yaffle commented 6 months ago

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