Decimal constructor doesn’t use the requested precision when converting from a number

[jrus]

It doesn’t seem to matter how many digits of precision I request; the decimal number with the fewest digits that is within half an ulp of the given floating point number is used, rather than the decimal number which precisely represents the floating point number to the requested precision.

For example:

Decimal.set({ precision: 100, rounding: 6 });

'' + Decimal(Math.PI);
=> "3.141592653589793"

I only got 16 digits when I asked for up to 100. But Javascript’s Math.PI is literally 884279719003555/2**48. If I am more careful I get:

'' + Decimal(Math.PI * 2**48).div(Decimal(2**48));
=> "3.141592653589793115997963468544185161590576171875"

Notice this is now an exact representation of the double-precision number Math.PI (with 49 digits, 33 more than the naive attempt).

---

This matters for my particular use case, which is trying to use higher-precision arithmetic to double-check the results of some careful numerical algorithms. The slight perturbations here of up to half an ulp can make a big difference in edge cases.

[MikeMcl]

Setting the precision has no effect on the precision of an instance returned by the constructor, and the value of an instance is created from the argument's string value.

If you want to create an instance from the underlying binary value of a number you can use

pi = Decimal(`0b${Math.PI.toString(2)}`)
// or
pi = Decimal(Math.PI.toFixed(50));

If you want the exact value of Pi to 100 digits you can use

Decimal.set({ precision: 100 });
pi = Decimal.acos(-1);

[jrus]

value of an instance is created from the argument's string value

This seems like the wrong thing to do when the argument is a number. Why not do something like `0b${input.toString(2)}` in the constructor?

---

Edit: or more specifically this might look more like

(x) => {
  if ((typeof x === "number") && isFinite(x)) {
    if (x > 0) return Decimal('0b' + x.toString(2));
    if (x < 0) return Decimal('-0b' + x.toString(2).slice(1))
  }
  return Decimal(x);
}

---

Edit 2: This claim also doesn’t seem to even always be true. For example, passing in a bignum to Decimal throws an error rather than using the string value.

[MikeMcl]

You're welcome.

When the argument is a number, the value of an instance is created from the argument's string value.

[jrus]

You're welcome.

I don’t understand what you mean here.

Thanks for the sorta kludgy workaround, I guess? It’s definitely a bit less kludgy than my first try. (Perhaps it should be added to the documentation, if the code can’t change.)

One other possible implementation (or workaround) would be to use input.toPrecision for numbers. E.g.

pi = Decimal(Math.PI.toPrecision(Decimal.precision))

When the argument is a number, the value of an instance is created from the argument's string value.

Fair enough, but my point wasn’t really to nitpick the statement, and I did not deeply investigate the internals of the library.

The point of this bug report is: this seems like clearly incorrect behavior, inferior for every use case I can think of to the alternative. If people explicitly ask for n digits, the obvious thing to do is believe them and try to provide n digits rather than min(n, 15) digits or whatever. The current behavior is not mentioned in the documentation, and I doubt many people would have their code broken by the change.

There is even a potential cross-javascript-implementation compatibility problem with the current behavior, since Number.toString is implementation defined. The ECMAScript standard points out:

Note that k is the number of digits in the decimal representation of s, that s is not divisible by 10, and that the least significant digit of s is not necessarily uniquely determined by these criteria.

(Is there some performance problem with giving the full requested precision? I didn’t try benchmarking it or anything...)

But while we’re at it, the string value would be a better source to use when the input is a bignum, instead of choking and throwing an error.

---

I’m happy to spend some time digging into the guts of your library and write a patch if that would be helpful. But your preceding reply doesn’t sound super receptive to that.

[jrus]

Actually the safest for number inputs would probably be to replace https://github.com/MikeMcl/decimal.js/blob/master/decimal.js#L4370 with:

        return parseDecimal(x, v.toPrecision(53));

Then if fewer than 53 digits are needed, whatever preferred rounding method can be used, and the result should be predictable.

[MikeMcl]

No, it wouldn't be safe at all. I think you are confusing bits with decimal digits.

And the maximum argument to toPrecision is 100 which is far short of the number of decimal digits required to precisely represent the underlying binary value of some JavaScript numbers.

Sorry but I have neither the time or incentive to address your longer post above which is similarly ignorant.

See the README for some cautions regarding passing numbers to the constructor, and an example of passing a binary value to get the exact value of a number.

[jrus]

No, it wouldn't be safe at all. I think you are confusing bits with decimal digits.

Aha. I tricked myself. I was thinking only of double-precision numbers with a non-negative exponent. 1 + 1/2^n has at most n digits after the decimal point, because 10 = 2·5. For example 5/4 = 1.25 (2 digits), 17/16 = 1.0625 (4 digits), 2049/2048 = 1.0004882813 (11 digits), and so on.

But if we consider e.g. 2**(-100) * (1 + 2**-52) this of course needs more digits.

So for an exact result it’s probably necessary to explicitly get the number’s mantissa as an integer, construct a Decimal from that, then divide by the exponent. Though the Decimal(x.toPrecision(Decimal.precision)) workaround should work if we don’t care about the rounding mode for a newly ingested number.

Sorry but I have neither the time or incentive to address your longer
post above which is similarly ignorant.

Are you always so hostile and insulting to people trying to help your project?

You could instead e.g. say “I spent a lot of time building this project a few years ago, but nowadays my interests lie elsewhere and this change doesn’t seem important enough to spend my time thinking about, even if someone else is willing to write the patch. Sorry; feel free to fork the project if you need different behavior and a workaround wrapper doesn’t cut it for your use case.”

See the README for some
cautions regarding passing numbers to the constructor, and an example
of passing a binary value to get the exact value of a number.

Yes, I read this. The “potential loss of precision” described in that readme is a completely different problem: someone writing a number down in their code that can’t be represented as a JavaScript number, and then being surprised when the JS parser turns their code string into a number that evaluates to something different than they expected.

It doesn’t actually discuss this specific point. Nowhere does it mention that numbers are ingested using the result of toString.

My problem is not with explicit strings in my code being accidentally converted to numbers, but with quantities that are already numbers being silently rounded by up to 1/2 ulp even when I explicitly asked for 100 digits. The use case here is trying to do higher-precision arithmetic to double-check the results of numerical algorithms. It is important for such use cases that the inputs to the double-precision algorithm and the higher-precision algorithm are exactly the same, because even slight differences can in some cases become very significant.

For example, I was trying to check the area of a very skinny spherical triangle using various alternative formulas for computing spherical area. A perturbation of <1/2 ulp in each coordinate of each vertex from Decimal’s lossy constructor directly caused a 50% relative error in the computed area of the triangle I was looking at, even with 100-digit arithmetic. (By comparison, the double-precision algorithm had relative error of about 0.01%, compared to the exact result.) I found another triangle where the perturbation from the lossy Decimal constructor caused a sign change (if you like, ∞ relative error).

I can of course write my own x.toPrecision(53) wrapper once I knew what the problem was. But it was surprising, took me a while to debug, and made me doubt the correctness of the Decimal library until after I had investigated more carefully and satisfied myself that it was just a lossy constructor but subsequent arithmetic was okay.

[MikeMcl]

I was thinking only of double-precision numbers with a non-negative exponent.

Your solution would not work reliably for numbers with positive exponents either as almost all numbers over about 1e+53 are going to have over 53 significant digits. For example, Number.MAX_VALUE has 309.

...the Decimal(x.toPrecision(Decimal.precision)) workaround should work if we don’t care about the rounding mode for a newly ingested number.

Yes, or more reliably using toString(2) and then rounding the Decimal using toSignificantDigits.

For example, using simple helper functions:

const binary = n => (n < 0 ? '-0b' : '0b') + Math.abs(n).toString(2);

Decimal(binary(Math.PI)) + '';           // "3.141592653589793115997963468544185161590576171875"

// Round to a supplied precision or to Decimal.precision
Decimal.fromDouble = (n, prec) => Decimal(binary(n)).toSignificantDigits(prec ?? Decimal.precision);

Decimal.precision = 10;
Decimal.fromDouble(Math.PI) + '';        // "3.141592654"
Decimal.fromDouble(Math.PI, 50) + '';    // "3.141592653589793115997963468544185161590576171875"

However, using toPrecision may be significantly faster than using toString(2) as it avoids this library having to convert binary to decimal, therefore a more efficient solution may be to use toString(2) only when necessary, something like:

Decimal.fromDouble = (num, prec) => {
  const abs = Math.abs(num);
  return Decimal(abs > 1e+99 || abs < 1e-20
    ? (num < 0 ? '-0b' : '0b') + abs.toString(2)
    : num.toPrecision(100)
  ).toSignificantDigits(prec ?? Decimal.precision);
};

Nowhere does it mention that numbers are ingested using the result of toString.

Why did you assume otherwise? The README makes it obvious to the attentive reader, for example:

0.3 - 0.1                     // 0.19999999999999998
x = new Decimal(0.3)
x.minus(0.1)                  // '0.2'
x                             // '0.3'

Obviously, x would not have the value 0.3 if the underlying binary value was used, it would instead be 0.299999999999999988897769753748434595763683319091796875 and x.minus(0.1) would be 0.19999999999999998335 at default precision.

The whole point of using this libray is to avoid the 'errors' that come from using binary floating point. For the most part, the only time when it more useful to use the underlying binary value of a number is when you are analysing binary floating point or the results of its arithmetic, and in that case it is perfectly reasonable to be required to explicitly ask for that binary value using toString(2).

My problem is not with explicit strings in my code being accidentally converted to numbers, but with quantities that are already numbers being silently rounded by up to 1/2 ulp even when I explicitly asked for 100 digits.

You didn't explicitly ask for 100 digits. The precision documentation makes it clear that Decimal does not round the return value. The way to do it is simply to use toSignificantDigits after the Decimal is created, which is the safer and more flexible approach.

There is even a potential cross-javascript-implementation compatibility problem with the current behavior, since Number.toString is implementation defined.

There is no potential problem as long as the toString value of the number has 15 significant digits or less. And as the README makes clear:

If using values with more than a few digits, it is recommended to pass strings rather than numbers to avoid a potential loss of precision.

Anyway, if I wanted this library to create a Decimal's value from the underlying binary value of a number I wouldn't use toString(2) or toPrecision internally. It is just a matter of base conversion using simple arithmetic. There is an implementation in an adapted version of this library here.

But while we’re at it, the string value would be a better source to use when the input is a bignum, instead of choking and throwing an error.

I don't know what you mean by "bignum", and we have to be careful of just calling toString on arbitrary objects, otherwise for example:

x = [123];
Decimal(x) + '';    // '123'

[jrus]

Yes, or more reliably using toString(2) and then rounding the Decimal using toSignificantDigits.

For example, using simple helper functions:

const binary = n => (n < 0 ? '-0b' : '0b') + Math.abs(n).toString(2);

This is a good start, but doesn’t quite work for Infinity, NaN, or -0. Maybe a Decimal.fromDouble method would be a valuable addition to the library, because getting it right for every edge case has some subtleties.

It also gets very big with extremely large or small exponents, e.g. binary(2**1023).length == 1026 is a pretty long string of mostly zeros. I don’t know how much effect that has on performance; maybe it’s not common enough to be a serious worry.

Nowhere does [the README] mention that numbers are ingested using the result of toString.

Why did you assume otherwise? The README makes it obvious to the attentive reader,

My experience is that every other mathematical/numerical library that takes in floating point input uses the floating point number passed in. I have never in my life encountered another library which first converts the floating point number to a string and then parses the string as a new number.

No first-time user of any library is going to be so “attentive” to the documentation that they will carefully parse every example hunting for clues about ways the internal implementation might violate their basic expectations, and end up perfectly inferring how the library works.

It would be much clearer with an explicit comment pointing out that new Decimal(0.3) == new Decimal("0.3") even though mathematically 0.3 cannot be represented by binary floating point numbers, because internally the constructor begins by asking the browser to convert a number to a string.

Obviously, x would not have the value 0.3 if the underlying binary value was used, it would instead be 0.299999999999999988897769753748434595763683319091796875 and x.minus(0.1) would be 0.19999999999999998335 at default precision.

Yes, the result of 0.19999999999999998335 is what I would expect, and if it did work that way, this example would be a good teaching opportunity about the benefit of passing in strings rather than numbers.

I hadn’t noticed that this example demonstrated the constructor rounding number inputs; I skimmed past it because I would expect such a use to stick to string inputs to the constructor. Also this example is explicitly supposed to be demonstrating “Decimal instances are immutable in the sense that they are not changed by their methods” rather than anything about what happens to numeric input to the constructor per se.

The whole point of using this library is to avoid the 'errors' that come from using binary floating point.

I thought the whole point of the library was to do decimal arithmetic with some specified k digits of precision, not to absolve users from understanding how computer arithmetic works.

You didn't explicitly ask for 100 digits. The precision documentation makes it clear that Decimal does not round the return value. The way to do it is simply to use toSignificantDigits after the Decimal is created, which is the safer and more flexible approach.

The Decimal constructor did round the value. Except instead of rounding to 100 digits, it rounded to ~15 digits. It would also probably be helpful for the documentation (starting with the README) to make it even more explicit that the Decimal constructor doesn’t use precision for rounding. Including Decimal in the list of functions that don’t use precision is not something people are going to notice unless they are specifically looking for it.

There is even a potential cross-javascript-implementation compatibility problem with the current behavior, since Number.toString is implementation defined.

There is no potential problem as long as the toString value of the number has 15 significant digits or less.

The problem is that Decimal(x) on one browser may sometimes return a different result than Decimal(x) on a different browser, for the same x. (I don’t know if this actually happens; just the ECMAScript spec allows it.)

Such seemingly trivial cross-platform differences have caused hard-to-track (and sometimes practically significant) bugs before, when someone used a library/function they believed to be deterministic and completely specified and it turned out there was a subtle non-specified difference they never considered.

But while we’re at it, the string value would be a better source to use when the input is a bignum, instead of choking and throwing an error.

I don't know what you mean by "bignum",

Sorry, I mean a bigint, a built-in object now included in the current version of all major browsers https://caniuse.com/bigint

Decimal(1n)
=> Error: [DecimalError] Invalid argument: 1

and we have to be careful of just calling toString on arbitrary objects, otherwise for example:

Fair enough. And the README is explicit here: “...constructor function, Decimal, which expects a single argument that is a number, string or Decimal instance”.

Adding quick support for ingesting bigints probably still a good idea. Even if it’s just checking for type 'bigint' and then returning parseDecimal(v.toString()) or the like.

[MikeMcl]

This is a good start, but doesn’t quite work for Infinity, NaN, or -0.

You missed -Infinity. Clearly, the function assumes n is finite, the same as it assumes n is of type number. For simplicity, I chose to follow toPrecision and not keep the sign of minus zero, but if required n < 0 would instead be 1 / n < 0.

The following can be inserted into Decimal.fromDouble to handle the above if required:

  if (typeof num != 'number' || !isFinite(num) || num === 0) return Decimal(num);

Maybe a Decimal.fromDouble method would be a valuable addition to the library, because getting it right for every edge case has some subtleties.

I have considered that possibility before but I have not yet felt compelled to include it, particularly as the binary value is readily available from toString(2), toExponential, toFixed or toPrecision.

It also gets very big with extremely large or small exponents, e.g. binary(2**1023).length == 1026 is a pretty long string of mostly zeros. I don’t know how much effect that has on performance; maybe it’s not common enough to be a serious worry.

It's worth noting that such values can be created using binary exponential notation for example:

Decimal('0b1p-1074').toNumber() === Number.MIN_VALUE;    // true

My experience is that every other mathematical/numerical library that takes in floating point input uses the floating point number passed in.

Many such libraries represent values internally in a power-of-2 base which makes it simple and fast to convert the binary value of a number to the value stored. Here, values are represented in a power-of-ten base which makes it more complex and slower. Another reason why I am not keen to promote the use of this library for analysing binary floating point.

I have never in my life encountered another library which first converts the floating point number to a string and then parses the string as a new number.

It can equally be considered to be the other way around. With, for example, Decimal(0.1) it is surely 0.1000000000000000055511151231257827021181583404541015625 that is more the "new number" than 0.1.

I think most users of this library would just expect Decimal(0.3) to return a Decimal with the value 0.3. This library has been downloaded millions of times a week for years and I am not getting flooded with complaints that a Decimal is created from the toString value of a number.

No first-time user of any library is going to be so “attentive” to the documentation that they will carefully parse every example hunting for clues about ways the internal implementation might violate their basic expectations, and end up perfectly inferring how the library works.

Yes, most users will actually test out the basic functionality for themselves rather than steaming ahead with their own cocksure assumptions. It is very discoverable how Decimal parses numbers and also that it does not round them to Decimal.precision.

It would be much clearer with an explicit comment pointing out that new Decimal(0.3) == new Decimal("0.3")

Well, it would be new Decimal(0.3).equals('0.3') but anyway there is something similar as the very first example in the README:

x = new Decimal(123.4567)
y = new Decimal('123456.7e-3')
z = new Decimal(x)
x.equals(y) && y.equals(z) && x.equals(z)        // true

The fact that the numeric literal's last fraction digit is 7 makes it pretty clear that the value of x is literally 123.4567 as that is not a value that is going to be able to be represented exactly in binary floating point.

I am not sure but I think there did use to be a note here that Decimal values were created from a number's toString value similar to this extract from the bignumber.js README:

When creating a BigNumber from a Number, note that a BigNumber is created from a Number's decimal toString() value not from its underlying binary value. If the latter is required, then pass the Number's toString(2) value and specify base 2.

new BigNumber(Number.MAX_VALUE.toString(2), 2)

I think I removed it or did not include it here because I thought it would be more of a challenge to some users' preconceptions for them to discover it for themselves.

The problem is that Decimal(x) on one browser may sometimes return a different result than Decimal(x) on a different browser, for the same x.

Here, the value of x is its toString value. In the vanishingly unlikely event that the same number had a different digit in the last place, then it's a different x. Anyway, as stated previously, using strings is the recommended approach.

Adding quick support for ingesting bigints probably still a good idea. Even if it’s just checking for type 'bigint' and then returning parseDecimal(v.toString()) or the like.

Yes, it's already an open issue #181

[MikeMcl]

Anyway, Jacob, thanks for the feedback. Your opinions have some merit and have been noted. The door is always open for a pull request - although it may not be dealt with immediately.