Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modulo operator does not conform to standard Python behavior #207

Open
avdstaaij opened this issue Jan 24, 2023 · 3 comments
Open

Modulo operator does not conform to standard Python behavior #207

avdstaaij opened this issue Jan 24, 2023 · 3 comments
Assignees

Comments

@avdstaaij
Copy link

In Python, the result of a % b for integers a and b always has the same sign as b. Numpy follows this convention as well. PyGLM, however, does not follow this rule for integer vectors. On my machine:

>>> -3 % 2
1

>>> np.array([-3]) % 2
array([1])

>>> vec2(-3,-3) % 2
vec2(1,1)

>>> ivec2(-3,-3) % 2
ivec2(-1,-1)

I assume the sign is unspecified for pyGLM in the last case.

I've looked in GLM's documentation of the modulo function. While it states that floating point modulo should behave like it does in Python, it does not actually describe an integer modulo operation at all, so it seems like pyGLM is free to choose what to do here.

I would like to request integer modulo to behave in the standard Python way (the same way as floating point modulo). This would reduce confusion and prevent subtle bugs.

@Zuzu-Typ
Copy link
Owner

This is a bit of a tricky topic.

In general, a mod operation and the corresponding div operation need to satisfy the following conditions:

q = a div b
r = a mod b

a == b × q + r
|r| < |b|

You are correct that GLM does not define a modulo for int vectors, however, it does define a division function for int vectors.
Thus the corresponding mod function needs to satisfy the condition above.
C (and therefore PyGLM) uses truncated division for int vectors. I.e. the result of a division is always rounded towards zero.
As a result of this, the remainder always has the same sign as the dividend (in this case a).

The issue here is that python defines two different div operations (/ and //), but only one mod operation (%). So the mod operation can only comply to one of the two.

I find that it's more reasonable that % corresponds to /, than //.

Floored division and it's respective modulo also take a bit longer to compute.

I believe there should at least be a method to do the floormod operation on int vectors.

@avdstaaij
Copy link
Author

Hmm, I suppose you're right. I was aware of the different interpretations of modulo, but I hadn't considered that it should match the division function.

It might have been more "Pythonic" to make integer / produce a float vector like it does for regular Python floats. Then % could have been implemented to match //. But like you said, you would then lose the division function as specified by GLM, and lose some performance. And of course, changing this now would break compatibility. I suppose it makes more sense to follow the GLM specification then the Python convention.

It might be worth it to describe this edge case in the wiki, since this behavior may be unexpected to Python developers.

@Zuzu-Typ
Copy link
Owner

Yes, you're right. A note in the documentation is the least I should do.

Perhaps I should also add dedicated truncdiv, floordiv and their respective mod counterparts tuncmod, floormod functions, as well as a truediv (i.e. result of division is a float or double vector) function so people have the option to choose the right operation in a verbose fashion.

@Zuzu-Typ Zuzu-Typ self-assigned this Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants