Potential bug in mod()

[LloydLS]

Hi,

Describe the bug
mod() sometimes returns an unexpected result, presumably due to round-off errors.

To Reproduce

Please compare:

mod(0.15, 0.05) 
// Actual result: 0.04999999999999999; 
// Expected result: 0

with

0.15 - 0.05 * floor(0.15 / 0.05) == 0
// Actual result: true
// Expected result: true

Version tested: 11.8.0

Best regards,

[josdejong]

Thanks for reporting. This is caused by the implementation of mod using Math.floor, see:

mathjs/src/plain/number/arithmetic.js

Lines 159 to 171 in ebf9287

	export function modNumber (x, y) {
	if (y > 0) {
	// We don't use JavaScript's % operator here as this doesn't work
	// correctly for x < 0 and x === 0
	// see https://en.wikipedia.org/wiki/Modulo_operation
	return x - y * Math.floor(x / y)
	} else if (y === 0) {
	return x
	} else { // y < 0
	// TODO: implement mod for a negative divisor
	throw new Error('Cannot calculate mod for a negative divisor')
	}
	}

To fix this for real, we should pull in the higher level function floor of mathjs, which uses config.epsilon. Maybe best is to keep the low level number function as is, copy the logic into mod.js, and adjust it there. Or maybe we should create a new internal helper function function modNumberInternal(x, y, floor) {...} so you can pass a different implementation for floor to it.

The same issue occurs with xgcdNumber which also uses Math.floor.

Anyone interested to help out improve mod and maybe xgcd? We should probably think it through a bit more before implementing a fix.

[Madhu003]

@LloydLS Can I work on it?

[josdejong]

Thanks for your offer Madhu, that would be great.

[josdejong]

Thanks for your PR #2940 @Madhu003 . 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?

[praisennamonu1]

Hi, @josdejong

I've gone through the issue and improved the precision logic of mod using epsilon (@LloydLS shouldn't have an issue anymore). But concerning your requirements, I need some clarifications:

• Do you want to ignore the .mod() method inherited from the decimal.js library used in BigNumber implementations in mod.js and gcd.js?
• Would it be okay if BigNumber implementation isn't configured via epsilon?

Expecting your reply.

[josdejong]

Thanks for your PR Praise! About your two questions: I think the .mod() method of decimal.js works fine indeed. The most important reason that we would like to implement our own version is to make the behavior of mod consistent with all related mathjs functions like floor, largerEq, etc. Those use epsilon, and hence they give slightly different results for edge cases with round-off errors. Does that make sense you think?

On a side note: we do want to implement a separate epsilon for bignumbers, since they obviously operate with a different precision than numbers, see #2608.

[josdejong]

Fixed now in v11.11.1