-
Notifications
You must be signed in to change notification settings - Fork 88
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
Change mod for Int #161
Comments
Hmm interesting, as doing this would break our current
vs
|
We can have this version of We’d just have to change |
See https://en.m.wikipedia.org/wiki/Modulo_operation. The suggested version is what is called “Euclidean division” on that page. Arguably that’s the most senate option given that the class is called |
*sensible |
I agree the proposed mod seems more sensible, I was just pointing out it's problematic for the rules it's supposed to be abiding by 🙂 The revised div seems extremely surprising to me, I'd probably rather we reconsidered the laws yet again than have that as the behaviour as I doubt it's what most people would assume for the term of "division" (a good example of why "friendly" names aren't great 😉). |
Oops, sorry, I made a mistake - we’d actually have the revised I disagree, I think this is an entirely sensible notion of division. It’s only that most programming languages do it the silly way, in that |
I recommend reading http://a-guide-to-the-purescript-numeric-hierarchy.readthedocs.io/en/latest/, specifically the chapters on the Euclidean algorithm, on polynomials, and on Euclidean rings to see how this is a very natural and powerful way of generalising the idea of integer division. |
You don't have to convince me that doing the lawful thing is right, I just worried about assigning not-most-layman-obvious behaviours to operations that everyone thinks they understand already. We have very few of those, just the arithmetic ones really. Having said all that, |
Yep, |
It's worth noting that once you have chosen either of a `mod` b = a - (a `div` b) * b (or equivalent). Alternatively, suppose you've chosen your (a `div` b) * b = a - a `mod` b holds, and since we are working in an integral domain (this is a prerequisite to have a There's one more alternative we should probably consider, which is where division rounds towards negative infinity. This has the property that That leaves us with:
where I'm using the same names as Boute does in his paper which discusses this exact topic [1]. (I accessed it through my university; I'm not sure how to access it if you're not a student, unfortunately.) The first thing to note is that all three definitions are equivalent when both divisor and dividend are nonnegative (if I write The only thing going for the T-definition is that it has the property that Both the E- and F-definitions have the useful property that For me personally, this means that (imo) we should only be considering the E- and F-definitions for the default The F-definition has one advantage over the E-definition in that plenty of existing programming languages implement it already; for example, Haskell ( I would be happy with either the E- or F- definitions. The suggested FFI code yields the F-definition. I think my preference would be the E-definition, but I'll mull it over. [1]: "The Euclidean definition of the functions div and mod", Raymond T. Boute, ACM Transactions on Programming Languages and Systems, Vol 14, No. 2, April 1992, pages 127-144. |
Thanks @hdgarrood for this amazing research. I'm also leaning towards E based on what you've presented, and agree that E and F are both better than T. |
E appeals to me too, I wonder why F is apparently more common. There is one slight problem with whatever we do here: we'll need to encode/update the behaviour in the inliner too I think. |
I guess we should consider doing this now? I'm not too sure how to deal with it on the compiler side of things, since it will be a bit weird if using the 0.12 compiler with Prelude v3.x behaves differently than when using 0.11. |
I think in this case all we can do document the behaviour change. If people are expecting things to just work across major versions without checking what's changed, they're likely in for a bad time. Not sure there's anything better? |
That works for me, hopefully most people will be updating library dependencies along with the compiler, and I guess in some instances that'll be required even. Plus this is somewhat undefined behaviour right now, and only affects |
Looking at the various options again, I think the E definition is probably the easiest to implement too, since div behaves that way already with the JS we're using for integers (
(That's right for E - it'd be edit: moved sign to RHS |
Not quite - the JS version is the T-definition, and T happens to coincide with E when the dividend is positive. However with negative dividends they won’t match. For example, you’ll have -2/3 being -1 for the E-definition but 0 for the T-definition. |
Hmm, I see 🤔 |
Would you mind if I take what you've done so far and build on it in a separate PR? I think this change is probably important enough that it deserves its own PR. |
Certainly, please do! I'm sure you have a better understanding of this than I do, just thought I'd get something moving with it. |
Also provide `quot` and `rem`, like Haskell does, for users who do want truncating division - the one which matches what JS does. I've temporarily exported `intDiv` and `intMod` so that I can use those in the tests and the compiler won't 'inline' different definitions of them; we'll want to modify the compiler to change this before merging.
Switch to Euclidean division for Int, resolves #161
Closing this as it's resolved in the 0.12 branch. |
For example, -3 mod 9 should be 6 (since 6 - 9 = -3) but in PS we have
mod (-3) 9 == (-3)
, which ultimately stems from JS's-3 % 9 === -3
Some people suggest using the following FFI:
References:
http://www.seqmedia.com/2011/03/16/javascript-negative-numbers-modulus-bug/
https://stackoverflow.com/questions/4467539/javascript-modulo-gives-a-negative-result-for-negative-numbers
The text was updated successfully, but these errors were encountered: