Skip to content

Conversation

GULPF
Copy link
Member

@GULPF GULPF commented Jan 21, 2018

This might just be a pointless breaking change, but it annoys me that modulus for floats uses floor division, when div and mod for integers uses truncating division.

@krux02
Copy link
Contributor

krux02 commented Jan 22, 2018

Yes this changes an unnecessary inconsistency. Here is an example how it is now.

import math

echo -3 mod 2
echo -3.0 mod 2.0

output:

-1
1.0

But I am still agains this changes, I would prefer a lot if integers would behave like the float in this case and the result would be:

1
1.0

I only need floor divison modulo. Never needed the truncating divison. Most of the time I use modulo is for indices of arrays arr[i mod arr.len]. But this patters dos not work for i < 0. So I end up writing this ugly code arr[(i mod arr.len + arr.len) mod arr.len] or I wrtie this: assert(arr.len-1 and arr.len == 0) #[ arr.len is power of two #] ; arr[i and (arr.len - 1)]. I really wish I could reliably write arr[i mod arr.len] and it would work reliably for negative numbers.

@GULPF
Copy link
Member Author

GULPF commented Jan 22, 2018

Here's a good read on the different ways to implement div and mod

I don't really have an opinion on floor div vs trunc div, but I doubt changing the behavior of div and mod for integers would be accepted since it will break a lot of code.

@data-man
Copy link
Contributor

data-man commented Jan 22, 2018

C++ std::fmod

fmod(+5.1, +3.0) = 2.1
fmod(-5.1, +3.0) = -2.1
fmod(+5.1, -3.0) = 2.1
fmod(-5.1, -3.0) = -2.1
fmod(+0.0, 1.0) = 0
fmod(-0.0, 1.0) = -0
fmod(5.1, Inf) = 5.1
fmod(+5.1, 0) = -nan

C++ std::remainder

remainder(+5.1, +3.0) = -0.9
remainder(-5.1, +3.0) = 0.9
remainder(+5.1, -3.0) = -0.9
remainder(-5.1, -3.0) = 0.9
remainder(+0.0, 1.0) = 0
remainder(-0.0, 1.0) = -0
remainder(5.1, Inf) = 5.1
remainder(+5.1, 0) = -nan

Nim's output:

import math

echo +5.1 mod +3.0
echo -5.1 mod +3.0
echo +5.1 mod -3.0
echo -5.1 mod -3.0
echo +0.0 mod 1.0
echo -0.0 mod  1.0
echo 5.1 mod Inf
echo +5.1 mod  0

2.1
0.9000000000000004
-0.9000000000000004
-2.1
0.0
0.0
nan
5.1

@GULPF GULPF force-pushed the float-mod-trunc branch 2 times, most recently from fdb45df to 04dffae Compare January 23, 2018 08:46
@GULPF
Copy link
Member Author

GULPF commented Jan 23, 2018

I changed the PR to simply use use fmod directly for C and % for JS, this is probably the most sane alternative.

@krux02
Copy link
Contributor

krux02 commented Jan 23, 2018

and this is what mod does in maxima, a computer algebra system (mathematically correct):

(%i8) mod(+5.1, +3.0);
(%o8)                          2.099999999999999
(%i9) mod(-5.1, +3.0);
(%o9)                         0.9000000000000004
(%i10) mod(+5.1, -3.0);
(%o10)                       - 0.9000000000000004
(%i11) mod(-5.1, -3.0);
(%o11)                        - 2.099999999999999
(%i12) mod(+0.0,+1.0);
(%o12)                                0.0
(%i13) mod(-0.0,+1.0);
(%o13)                               -0.0
(%i14) mod(5.1, inf);
                                             5.1
(%o14)                       5.1 - inf floor(---)
                                             inf
(%i15) mod(5.1, 0);
(%o15)                                5.1

To be honest there are multiple definitions of mod.

@krux02
Copy link
Contributor

krux02 commented Feb 23, 2018

I have changed my mind. I agree that this PR should be taken.

@GULPF can you add an entry in the changelog.md?

@Araq
Copy link
Member

Araq commented Feb 24, 2018

This needs a changelog entry.

@krux02
Copy link
Contributor

krux02 commented Feb 28, 2018

I like how it is now. When the tests are fixed. I would approve it.

@GULPF GULPF force-pushed the float-mod-trunc branch from e032c48 to c67c70a Compare March 18, 2018 10:50
@GULPF
Copy link
Member Author

GULPF commented Mar 18, 2018

Ready to merge

@GULPF GULPF force-pushed the float-mod-trunc branch from 3981d46 to 1610945 Compare April 13, 2018 06:19
@GULPF GULPF force-pushed the float-mod-trunc branch from e46010a to b0119a9 Compare May 29, 2018 20:13
@data-man
Copy link
Contributor

Changelog's entries for floorDiv/floorMod?

@data-man data-man merged commit 65070a6 into nim-lang:devel May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants