torch / torch7

http://torch.ch
Other
8.97k stars 2.38k forks source link

Possible bug in randperm () function #966

Closed viktorheli closed 7 years ago

viktorheli commented 7 years ago

Possible bug in randperm() function. If I use torch.setdefaulttensortype ( 'torch.FloatTensor') randperm sometimes returns zero. th> torch.randperm(3) 1 3 2 [torch.DoubleTensor of size 3]

th> torch.setdefaulttensortype('torch.FloatTensor') [0.0000s] th> torch.randperm(3) 0 2 1 [torch.FloatTensor of size 3]

The documentation says it returns a value from 1 to the specified value.

allskyee commented 7 years ago

I narrowed down the issue to pkg/torch/lib/TH/generic/THTensorMath.c lines 567~576 and made a small progress.

Specifically line 572. This line is only executed when default type is float and seems to fail to add 1.

Making it so that the if condition on line 571 is always false so that line 574 is always executed seems to solve the problem for me.

So rather than executing 572 TH_TENSOR_APPLY2CONTIG(real, r, real, t, THVector_(adds)(r__data, t_data, value, r__len););

do 574 TH_TENSORAPPLY2(real, r, real, t, r__data = t_data + value;);

adamlerer commented 7 years ago

Probably fixed by https://github.com/torch/torch7/pull/963.

viktorheli commented 7 years ago

Thank you all. I have successfully recompiled Torch with fix from the Issue 963. Everything works fine.

th> torch.randperm(3) 2 1 3 [torch.DoubleTensor of size 3]

                                                                  [0.0002s]

th> torch.setdefaulttensortype('torch.FloatTensor') [0.0001s] th> torch.randperm(3) 2 3 1 [torch.FloatTensor of size 3]

apldeap commented 7 years ago

Not an issue but still worth mentioning. If using ByteTensor as default tensor type, passing values greater than 255 to randperm will result with tensor containing zero as well:

th> torch.setdefaulttensortype('torch.ByteTensor') th> s1 = torch.randperm(256) th> s1:min() 0

niuxch commented 7 years ago

There seems still issues in the randperm() function, when setting "torch.setdefaulttensortype ( 'torch.FloatTensor')".

Here is an example: th> torch.setdefaulttensortype( 'torch.FloatTensor' ) th> torch.randperm(33806495):max() 33806496

pavanky commented 7 years ago

@niuxch That's a floating point issue. Pretty sure 33806495 gets rounded up to 33806496 when using 32 bit floats.

In fact,here's a simple test in c:

$ cat float_test.c && gcc float_test.c -o float_test && ./float_test
#include <stdio.h>

int main()
{
    float val = 33806495;
    printf("%f\n", val);
    return 0;
}
33806496.000000

Here's a wiki article on floating point precision for integers: https://en.wikipedia.org/wiki/Single-precision_floating-point_format#Precision_limits_on_integer_values

The number you passed in lies in the "multiples of 2" range.