Potential Bug in mod()

[praisennamonu1]

I think we need to think through some steps before we can finish up this PR:

1. I think it is essential to have this behavior configured via epsilon

2. We have to think through how to configure and pass this epsilon, both in the case of the plain number implementation as well as the generic implementation, in such a way that the code base as a whole works consistent.

   1. I think we should keep all plain number functions as is, without handling round-off errors (and corresponding epsilon etc).
   2. And we should handle round-off errors in all generic implementations (all relational functions, and mod and gcd). At this level we do have epsilon so we can easily use that.

3. Concretely: I think the most pragmatic solution is to copy the code of modNumber and gcdNumber into mod.js and gcd.js, and then change the implementation to handle round-off errors in a similar way as function larger using the util function nearlyEqual(x, y, epsilon).

4. We will also need to change the BigNumber implementations of mod.js and gcd.js to handle round-off errors (similar to for example function larger and other relational functions.

5. We'll need to write unit tests to validate the new behavior.

What do you think?

To solve issue #2936 raised and based on these requirements written by @josdejong, I've made changes to mod.js and gcd.js files and BigNumber implementation.

I've also added test cases to validate the new behaviour

[josdejong]

Thanks @praisennamonu1 for your PR fixing the rounding issues with mod.

I made a few inline comments, can you have a look at those?

[praisennamonu1]

Hi @josdejong
Can you go through my recent commit?
You mentioned something about the BigNumber implementation which I didn't quite get but I hope it's the way it should be?

[josdejong]

Nice 😎! This is a very elegant solution.

You've resolved my remark about BigNumber by using floor there too. (we could go a step further and merge _gcdNumber and _gcdBigNumber into one but that is out of scope for this PR I think, unless you prefer to do that on the fly).

I have one last remark: you have now extended src/function/arithmetic/mod.js to support negative divisors. Can you also change modNumber in src/plain/arithmetic.js to have the same behavior (except using Math.floor there) and add unit tests for it?

[praisennamonu1]

Thanks!

I've made changes to the mod implementation in arithmetic.js but since it's not being imported to be used, there's no way actually to add unit tests to validate it.

I will look into the gcd implementation to see if they can be combined. Because BigNumber and Number are different, from what I can understand.

[josdejong]

I've made changes to the mod implementation in arithmetic.js but since it's not being imported to be used, there's no way actually to add unit tests to validate it.

Ah, you're right, the /src/plain functions do not have unit tests since they where all covered by the higher level implementations in src/function. I think you can create a new test file /test/node-tests/plain/number/arithmetic.js and add a test for the function gcd from src/plain/number/arithmetic.js there.

Please let me know if you need help.

[praisennamonu1]

Been a while, but I've added the tests for the function in the new file.

Could you check that out?

[josdejong]

Thanks for the updates @praisennamonu1 , this looks good and well tested👌

I see mentioned putting the new unit tests under /test/node-tests/plain/number/arithmetic.js, that should be /test/unit-tests/plain/number/arithmetic.js. I'll move the file after merging so we can finish and publish this fix now.

[josdejong]

The fix is published now in v11.11.1, thanks again Praise!