RubixML / Tensor

A library and extension that provides objects for scientific computing in PHP.
https://rubixml.com
MIT License
229 stars 28 forks source link

modColumnVector remainder behaviour fix #2

Closed henrique-borba closed 4 years ago

henrique-borba commented 4 years ago

modColumnVector does not return a correct result and PHPUnit is also expecting the wrong values. The true mod (checked using NumPy) for the implemented test (Tests/MatrixTest::modColumnVector) is:

[
     [ 2. ,  0.5,  2. ],
     [-0. , -0. , -0. ],
     [ 0.8,  3.6,  0.6]
 ]

PHP modulus operand "%" is made for integer only operations, witch can cause unexpected results when used for floating points.

fmod for some reason also failed to return the correct value and it is also famous for having problems with floating points, so it had to be implemented manually.

andrewdalpino commented 4 years ago

Interesting! Nice catch @henrique-borba

Why is PHP's fmod() incorrect? and how much is it off by? Or is it just not compatible with integers?

Should we do the same to modRowVector() and modMatrix()? For both Matrix and Vector classes?

Also, we have matching methods in Zephir for the extension .. should those be changed as well?

henrique-borba commented 4 years ago

Interesting! Nice catch @henrique-borba

Why is PHP's fmod() incorrect? and how much is it off by? Or is it just not compatible with integers?

Actually PHP fmod is just a simple call to C fmod:

/* {{{ proto float fmod(float x, float y)
Returns the remainder of dividing x by y as a float */
PHP_FUNCTION(fmod)
{
    double num1, num2;

    ZEND_PARSE_PARAMETERS_START(2, 2)
        Z_PARAM_DOUBLE(num1)
        Z_PARAM_DOUBLE(num2)
    ZEND_PARSE_PARAMETERS_END();

    RETURN_DOUBLE(fmod(num1, num2));
}

I found this:

Python has a "true" modulo operation, while C has a remainder operation.

It has a direct relation with how the negative integer division is handled, i.e. rounded towards 0 or minus infinite. Python rounds towards minus infinite and C(99) towards 0, but in both languages (n/m)*m + n%m == n, so the % operator must compensate in the correct direction. (https://stackoverflow.com/questions/1907565/c-and-python-different-behaviour-of-the-modulo-operation)

I guess you should implement fmod as a Tensor::remainder function instead of modulus. Anyway, you should avoid the modulus '%' operator because it is actually a remainder operator and it is designed for positive integers.

henrique-borba commented 4 years ago

Should we do the same to modRowVector() and modMatrix()? For both Matrix and Vector classes?

Also, we have matching methods in Zephir for the extension .. should those be changed as well?

Yes, then you implement remainder, remainderRowVector, remainderMatrix and remainderColumnVector with the same code but using fmod so you still have access to remainder operations when needed.

andrewdalpino commented 4 years ago

Ok sounds like a bit of busy work to do, I'll take care of that asap.

andrewdalpino commented 4 years ago

Implementing the rest of the changes in a separate commit

andrewdalpino commented 4 years ago

@henrique-borba What if we left mod() to mean 'integer modulo' and create an fmod() universal function to mean 'floating point modulo'?