mod1, fld1, rem1 broken

[mschauer]

julia> map(x -> int(mod1(uint(x),uint(5))), 0:15)'
1x16 Array{Int64,2}:
 5  1  2  3  4  5  5  1  2  3  4  5  1  2  3  4

It is like this since 0.1.2.

[GunnarFarneback]

This is indeed broken. mod1(x,y) doesn't work at all for unsigned x,y when x>y. Moreover fld1(x,y) gives rather less useful results for signed x<=0 and incorrect results for unsigned x=0.

julia> [rem1(k,5) for k=-10:10]'
1x21 Array{Int64,2}:
 0  1  -3  -2  -1  0  1  -3  -2  -1  0  1  2  3  4  5  1  2  3  4  5

julia> rem1(uint(0),uint(11))
0x0000000000000005

[ivarne]

The definition for mod1 is in base/operators.jl:115

mod1{T<:Real}(x::T, y::T) = y-mod(y-x,y)

For moderately large x where x > y, y - x underflows and the result will only be accurate for y = 2^n

[GunnarFarneback]

When I wrote fld1 above I actually meant rem1, as in the examples, but it's valid for fld1 too. Those are really only designed to be used for integer x > 0 and y > 0, in the context of 1-based indexing. The simplest solution for rem1 may be to document that it's only intended for x > 0 and otherwise refer to mod1. Or unexport it; the difference between rem and mod is how they behave for negative arguments and it's not clear that this distinction is interesting or even relevant for rem1/mod1.

As for the mod1 problem at the core of this issue there seems to be a very simple solution. It was broken by this commit with the comment

Also, in this commit: mod1 is now just as fast as mod.
That's a 213x improvement.

When I test it now I see no measurable speed difference, so just revert that change.

[ivarne]

We should at least add the old method definition for T <: Unsigned

[mschauer]

Btw, @StefanKarpinski did you see this?

[mschauer]

This should perhaps get a bug tag.

[jiahao]

Note: mod1, fld1, rem1 have no tests.

[eschnett]

This should be fixed by #14140: mod1 was corrected by #10585, fld1 now has a consistent behaviour for negative values, and rem1 is deprecated.

[mschauer]

Great