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

math.numeric(x, 'bigint') produces inconsistent/erroneous results for other custom numeric types. #3366

Open
gwhitney opened this issue Jan 27, 2025 · 1 comment · May be fixed by #3375
Open

Comments

@gwhitney
Copy link
Collaborator

Describe the bug
When x has a type other than string or number, math.numeric(x, 'bigint') does not necessarily succeed and when it does return a value, the value may not be correct.

To Reproduce

import math from './src/defaultInstance.js'
const bi = x => math.numeric(x, 'bigint')
bi(math.bignumber("12345678901234567890")) // returns 12345678901234567890n as expected
bi(math.bignumber("123456789012345678901234567890")) // throws `SyntaxError: Cannot convert 1.2345678901234567890123456789e+29 to a BigInt`
bi(12345678901234567890n) // throws `TypeError: Cannot convert 12345678901234567890 of type "bigint"; valid input types are string, number, BigNumber, Fraction`
//... even though it should be a no-op.
bi(3**50) // returns 717897987691852578422784n; actual value is 717897987691852588770249n
bi(27.4) // OK, throws `RangeError: 27.4 can't be converted to BigInt because it isn't an integer`
bi(math.bignumber(27.4)) // Throws `SyntaxError: invalid BigInt syntax` -- odd that it is a different error
bi(bignumber(1000000000000000027.4)) // Returns 1000000000000000000; but this is due to lack of safety in number => bignumber conversion, not really a problem with numeric()
bi(bignumber("1000000000000000027.4")) // Throws same error as bignumber(27.4); consistent with each other but not with number conversion
bi(fraction(27.4)) // Throws same error as bi(27.4)
bi(fraction("123456789012345678901234567890123456789/2")) // Returns 61728394506172842349823105903630483456n
// Oops that fraction is not an integer, and it's actually equal to bignumber("61728394506172839450617283945061728394.5")
// so it should throw, and moreover the result that is being produced is off in many digits.
bi(fraction("1234567890123456789012345678901234567890/2")) // Returns 617283945061728423498231059036304834560n
// So that fraction is an integer, so we should get an answer, but the actual value of that fraction is 617283945061728394506172839450617283945, so the result is incorrect even though Fraction is arbitrary-precision.

Discussion
All math types are being converted to bigint via BigInt(x). I believe that is the built-in BigInt, and I think that when x is not numeric it is converting to string and then from that to bigint (?). The BigNumber and Fraction classes were never designed to support the built-in conversion in this way via BigInt. We should instead have dedicated conversions for each of our custom numeric types.

I am comfortable with the conversions throwing when their arguments are not integers, but this should happen consistently.

The conversions should preserve the full precision of the argument, whatever it may be.

For conversions from number and BigNumber outside their respective ranges of being able to represent each distinct integer precisely, there is a question of what the behavior should be: (A) throw, (B) warn, or (C) silently return the integer nominally represented by that value, even though it may be a non-fully-precise result of an apparently exact integer calculation (like 3**50). I think the current behavior is (C) under the philosophy that this is an explicit, rather than implicit, conversion request. (If so, then there needs to be a way for the user to do a "safety check". That exists when the operand is number with JavaScript's MAX_SAFE_INTEGER; is there something corresponding for BigNumber?)

I will submit a PR fixing these issues, adhering to the apparent design decisions detailed above; if a different design is preferred (e.g. converting to the floor of the operand when it is non-integer, or (A) or (B) instead of (C) when the operand is outside a "safe integer" range), I can adjust the PR.

@gwhitney
Copy link
Collaborator Author

Note there are similar (but not quite identical) bugs in the "constructor" function math.bigint(). I plan to address them there and just call bigint() from numeric(x, 'bigint').

gwhitney added a commit to gwhitney/mathjs that referenced this issue Jan 31, 2025
   Adds options to the bigint constructor function `math.bigint()` to
   control whether/how non-integer inputs are rounded or errors thrown,
   and whether "unsafe" values outside the range where integers can be
   uniquely represented are converted. Changes `math.numeric(x, 'bigint')`
   to call the bigint constructor configured to allow unsafe values but
   throw on non-integers (closest approximation to its previous buggy
   behavior).

   Also restores documentation generation for constructor functions,
   and initiates History sections in function doc pages.

   Resolves josdejong#3366.
   Resolves josdejong#3368.
   Resolves josdejong#3341.
@gwhitney gwhitney linked a pull request Jan 31, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant