improve handling of precision issue in torch.multinomial (solves #4858)

[t-vi]

Hi,

I think I figured out #4858:
If you do torch.cumsum(freqs, 0), you see that the cumulated sum isn't one when the first of the final 0.0 probabilities happen, so there is a precision issue at the upper end of the range (making the breakage more subtle).
What seems to happen in aten/src/THC/THCTensorRandom.cuh is that the (0.0, 1.0] random value val generated by cuda_uniform is passed from sampleMultinomialWithReplacement to binarySearchForMultinomial in some cases (1.0 ?), the lt(midVal, val) seems to evaluate to true always and we end with start = size (start being the result of the binary search).
Then the if (start == size) catches this precision issue but does the wrong thing in setting start = 0. A more correct thing is to set start = size - 1 (but care for size = 0 unless the compiler figures it cannot happen) and let the code that follows find a non-zero-probability bin.

With the attached small fix @coventry's test case passes 1 million iterations when it used to run into an error reliably in < 10 000 iterations (and I checked that it was indeed reaching the start = 0 in this case).

I hope this helps.

Best regards

Thomas

[zou3519]

Thanks for the PR, @t-vi! Could you add a test case for this in test/test_cuda.py please?

[t-vi]

I can, but it won't actually catch the error reliably.
Edit: @zou3519 Would you want a test that tries a specific random seed that fails (which I would venture produces a "1.0" random number) or do you prefer to try 100k or 1M seeds?

[zou3519]

Is the problem that the CUDA RNG is returning a number in the range (0.0, 1] instead of [0, 1)?

In addition, it's fine if the test doesn't always catch the error, as long as it doesn't error out on correct code. I don't know how random number generation works on CUDA, but if you could fix a random seed that makes the random number 1, using that seed could be a test.

Minor comment below if we do go with this solution.

[zou3519]

Ah sorry, just read your comment @t-vi. I think a specific random seed that fails would be good. For the second case, if the test doesn't take a long time to run that would be good (but yeah it might take too long)

[t-vi]

So I have a random seed that does cause this, but I'm not sure how strongly it depends on specifics of the current setup of sampleMultinomialWithReplacement.
Trying 100k seeds takes ~70 seconds on my GTX1080, so it would seem to be a bit over the top.
However, the first error happens at 11042, so 10k seeds is not enough...

[ezyang]

@pytorchbot test this please

[ezyang]

@pytorchbot test this please

[ssnl]

LGTM