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

toNumber does not respect ROUNDING_MODE #360

Closed
bakkot opened this issue Dec 4, 2023 · 11 comments
Closed

toNumber does not respect ROUNDING_MODE #360

bakkot opened this issue Dec 4, 2023 · 11 comments

Comments

@bakkot
Copy link

bakkot commented Dec 4, 2023

Consider:

BigNumber.config({ DECIMAL_PLACES: 1000, ROUNDING_MODE: BigNumber.ROUND_HALF_UP });
console.log(BigNumber('9007199254740993').toNumber());

The mathematical value 9007199254740993 is precisely between two JS number values (9007199254740992 and 9007199254740994). So I would expect that when converting the BigNumber representing 9007199254740993 to a JS number value using the ROUND_HALF_UP rounding mode, it would round up. But it doesn't.

In fact, x.toNumber() appears to get the string representation of x and then rely on the JS engine to parse that string to a number, ignoring ROUNDING_MODE entirely.

When there are fewer than 20 significant digits, JS engines are required by the JS specification to use ROUND_HALF_EVEN ("This procedure corresponds exactly to the behaviour of the IEEE 754-2019 roundTiesToEven mode").

But when there are more than 20 significant digits, in some cases it's actually up to the engine which of the two nearest neighbors to use, so this isn't a well-defined operation.

@shuckster
Copy link

Type 9007199254740993 into a JavaScript REPL. What do you get?

@bakkot
Copy link
Author

bakkot commented Dec 5, 2023

JavaScript uses ties-to-even semantics. Here I've configured this library to use half-up semantics, and I expected that to apply to this operation.

@MikeMcl
Copy link
Owner

MikeMcl commented Dec 5, 2023

As per the documentation, x.toNumber() does not use ROUNDING_MODE and is the equivalent to Number(x).

@bakkot
Copy link
Author

bakkot commented Dec 5, 2023

Shouldn't it? Is there a reason it doesn't? It seems like if you're doing your operations with a particular rounding mode, and then trying to get a Number at the end, you'd probably want the rounding mode to apply to the process of getting the Number as well. I did, at least.

@shuckster
Copy link

Type 9007199254740993 into a JavaScript REPL. What do you get?

The reason I asked this is because the language itself cannot, using its native number-type, represent the number you wish to calculate.

To represent numbers that the native JavaScript number-type cannot, you must use strings and a library like BigNumber that can work with them.

The problem cannot be worked-around. If you want an accurate decimal number type (at this time) in JavaScript, it's strings or nothing.

@bakkot
Copy link
Author

bakkot commented Dec 5, 2023

I'm not asking to represent the mathematical value 9007199254740993 in JS. I'm asking that, when I represent that value using this library, and convert it to a JS number (which necessarily entails rounding), the conversion to a JS number should respect the rounding mode configured in this library.

@shuckster
Copy link

Press F12 to open the Chrome console.

Type the following:

9007199254740993 === 9007199254740992

If you can explain how a library like BigNumber.js is able to solve this problem, please let us know so we can write a Pull Request to fix it.

@bakkot
Copy link
Author

bakkot commented Dec 5, 2023

I know how numbers in JavaScript work. BigNumber('9007199254740993') is not a JavaScript number. As long as the library is configured with enough precision, it precisely represents the mathematical value 9007199254740993. It is perfectly possible to write an algorithm which converts that value to a JavaScript number under any choice of rounding mode.

@shuckster
Copy link

BigNumber('9007199254740993') is indeed not a JavaScript number, but you did say you want to convert it to one, right? In such a case, the library must consider more cases than just this single number.

Perhaps I'm missing the point, but the assumption I've been working under is that the whole purpose of BigNumber et al. is to avoid the pitfalls of IEEE-754 and MAX_SAFE_INTEGER.

Apologies, but I'm not very clear on the use-case of implementing such a library and then converting its results to the very thing we're trying to save ourselves from.

@MikeMcl
Copy link
Owner

MikeMcl commented Dec 6, 2023

I think it's a reasonable enhancement proposal on the face of it, but ultimately just isn't worth implementing.

Although straightforward, it would only make a difference when converting values with more than 15 significant digits that do not decimal-binary-decimal round-trip, so I am not convinced its limited utility justifies the extra code weight and implementation effort required. It would also be considerably slower than the current method as it would involve converting the value to binary before rounding.

There is also the issue of what should be the result of converting your example value, 9007199254740993, when the rounding mode is ROUND_HALF_EVEN, which is defined here as:

Rounds towards nearest neighbour. If equidistant, rounds towards even neighbour.

when 9007199254740992 and 9007199254740994 are both even!

@bakkot
Copy link
Author

bakkot commented Dec 6, 2023

Fair enough. Might be worth documenting how toNumber chooses which JS value to use when the true value is not precisely representable?

There is also the issue of what should be the result of converting your example value, 9007199254740993, when the rounding mode is ROUND_HALF_EVEN, which is defined here as:

True! I think of ROUND_HALF_EVEN as meaning IEEE's ties-to-even, which refers to the evenness of the significand (and so it would be 9007199254740992). But that's not quite the meaning here.

@bakkot bakkot closed this as completed Dec 6, 2023
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