fix: Handle bigints properly in more cases #3345
Conversation
* Allow a bigint to appear in a max/min of nonhomogeneous arguments
* If randomInt is called with a bigint or a pair of bigints, return
a bigint.
* Preserve uniformity of results if randomInt is called with a very large
range.
* Extend log, log2, and log10 to bigints
* Add tests for all of the above issues.
* Bonus: fix one JSDoc comment issue. If every PR fixes one, we will soon
get through the 150+
src/function/arithmetic/log.js
Outdated
| bigint: function (x) { | ||
| if (x > 0 || config.predictable) { | ||
| if (x <= 0) return NaN | ||
| const s = x.toString(16) |
There was a problem hiding this comment.
Can you explain how this coversion to hex and comparing lengths works?
Is there a way to move this logic in a helper function and use that in log, log10, and log2?
There was a problem hiding this comment.
Done, moved to src/utils/bigint.js. If you prefer a different file name, let me know.
| @@ -48,9 +50,11 @@ export const createRandomInt = /* #__PURE__ */ factory(name, dependencies, ({ ty | |||
| * @return {number | Array | Matrix} A random integer value | |||
| */ | |||
| return typed(name, { | |||
| '': () => _randomInt(0, 1), | |||
| '': () => _randomInt(0, 2), | |||
There was a problem hiding this comment.
Ouch, this was a bug in randomInt, right? Before it always returns zero.
There was a problem hiding this comment.
Correct, before it was a fancy constant function ;-)
Sorry about the delay in getting back on this, I was at a conference last week.
src/function/relational/larger.js
Outdated
| @@ -55,20 +62,28 @@ export const createLarger = /* #__PURE__ */ factory(name, dependencies, ({ typed | |||
| * @param {number | BigNumber | bigint | Fraction | boolean | Unit | string | Array | Matrix} y Second value to compare | |||
| * @return {boolean | Array | Matrix} Returns true when the x is larger than y, else returns false | |||
| */ | |||
| const bignumLarger = bigLarger(config) | |||
There was a problem hiding this comment.
I think the factory function bigLarger is not needed and we can just create a function bignumLarger here? Like:
function bignumLarger (x, y) {
return x.gt(y) && !bigNearlyEqual(x, y, config.relTol, config.absTol)
}There was a problem hiding this comment.
Agreed, that is simpler; refactored as suggested.
src/function/relational/smaller.js
Outdated
| @@ -55,20 +62,28 @@ export const createSmaller = /* #__PURE__ */ factory(name, dependencies, ({ type | |||
| * @param {number | BigNumber | bigint | Fraction | boolean | Unit | string | Array | Matrix} y Second value to compare | |||
| * @return {boolean | Array | Matrix} Returns true when the x is smaller than y, else returns false | |||
| */ | |||
| const bignumSmaller = bigSmaller(config) | |||
There was a problem hiding this comment.
Same as with the larger function: just put the returned function of bigSmaller here?
|
OK, I believe I have incorporated all feedback and gotten this PR back into a good state. Thanks for your review. |
smallerandlarger.Resolves #3344.