pydata/numexpr

Incorrect result with python % (modulo) operator and integer variable

arondaniel opened this issue · 8 comments

arondaniel commented

Python 3.9.13, numpy 1.23.0, numexpr 2.8.3

Hello and many thanks for a great library.

Using python modulo gives incorrect result if integer variable used w/ modulo operator.

solar = np.array([ -135 ])

Expected:

solar%360
array([225])

Incorrect answer using int variable:

ne.evaluate("solar%360")
array([-135], dtype=int64)

Workaround, make left or right side a float:

ne.evaluate("solar%360.")
array([225.])

27rabbitlt commented

According to the C code in interp_body.cpp, we could see that numexpr directly use operator% in C language. And in C/C++, -135 % 360 = -135.

I don't know choosing which one is better, but it's ugly to see this unconsistency between integer operation and float operation. It's easy to unify them, and I can fix this. But I don't know which one should we preserve. Do you have any suggestions? @robbmcleod

robbmcleod commented

In general NumExpr should produce the same output as NumPy. @27rabbitlt if you want to fix it, I have a discussion of the issue here starting on line 986:

https://github.com/pydata/numexpr/blob/numexpr-3.0/code_generators/interp_generator.py

With such heavy branching it's a little inefficient. If we had better support for unsigned integers in NE2.8 then it would be an option for the unsigned integer operation to remain performant.

27rabbitlt commented

@robbmcleod Yes, it's unperformant to add one more branch. Need some time to think about the solution carefully.

27rabbitlt commented

@robbmcleod It seems that we can use this kind of thing:

previous:

l_dest=l2 ? l1 % l2 : 0

updated:

l_dest=l2? (l1 % l2 + ((l1 > 0) ^ (l2 > 0)) * l2 : 0)

In my environment, the latter didn't create much overhead, the previous one costs 125ms on average with two np.array with size of 1e8 and dtype of np.int64, and the updated one costs 154ms on average.

robbmcleod commented

@27rabbitlt Your function does not seem to work if the operand is zero and the modulo is positive, e.g.

a = 0
n = 360
out = ne.evaluate('a % n')

out is 360 for NumExpr but 0 for NumPy and Python. Do you want to try again with nesting your current ternary in another one to cover this specific case?

I'd appreciate a pull request with a test, if you're game.

27rabbitlt commented

@robbmcleod sorry for late reply because I was preparing for GRE test 😭 I will try to fix it as soon as possible

robbmcleod commented

Should be fixed in b5c7c10. I used the following:

OP_MOD_III: VEC_ARG2(i_dest = i2 == 0 ? 0 :((i1 % i2) + i2) % i2);

which has two C % remainder ops, so possible performance loss, but it does pass all tests.

27rabbitlt commented