I can't call the random[T](x: Slice[T]): T #4353

Closed
ReneSac opened this Issue Jun 17, 2016 · 11 comments

Projects

None yet

6 participants

@ReneSac
Contributor
ReneSac commented Jun 17, 2016

How should it be called? I'm using int as a type as an example.

import random
echo random[int](low(int) .. high(int))

Gives me:

a4.nim(2, 6) Error: expression 'random' has no type (or is ambiguous)

While with:

import random
proc foo[T](): T = random[T](low(T) .. high(T))
echo foo[int]()

I get:

Traceback (most recent call last)
a4.nim(3)                a4
a4.nim(2)                foo
random.nim(91)           random
system.nim(2532)         sysFatal
Error: unhandled exception: over- or underflow [OverflowError]
@endragor
Contributor

In the first case, random module name somehow clashes with the random procedure call. For example, writing it this way makes it work:

import random as r
echo random[int](low(int) .. high(int))

The overflow occurs because of how the procedure is implemented - it relies on the difference of slice bounds being of the same type as the bounds. Subtracting low[T] from high[T] for integers creates overflow.

@ReneSac
Contributor
ReneSac commented Jun 19, 2016

That random module is a mess...

Module names in nim are usually in plural because of that... 'randoms' is not exactly the same thing though, but is there a better alternative?

And for the second issue, that is why signed integers shouldn't be at the core of random modules... and the test suit for random is really lacking.

Besides that, the way used to clamp the range produces biased and low quality results, and the randomize() function

And why is the range for random[T](x: Slice[T]) exclusive when the rest of the language uses inclusive ranges?

Anyway, we need non-backwards compatible fixes here...

@kirbyfan64
Contributor

and the test suit for random is really lacking

Well, to be fair, it's hard to test random numbers...

@ReneSac
Contributor
ReneSac commented Jun 19, 2016 edited

First, you should test if you can actually use the functions.

And now that I'm trying to fix things and running the test, I discovered that the only test there is completely bogus. Ok, it will compile and run if unaltered. But if I insert a call to randomize before the loop the test will fail most of the time...

EDIT: Ok, calling it totally bogus is too strong. It has it's uses. But it needs more iterations by table member to stabilize the values... 100 is too few. Now even with a 10% margin it rarely fails. But the test could be better anyway.

@dom96 dom96 added the Stdlib label Jun 19, 2016
@Araq
Member
Araq commented Jun 19, 2016

Hey, but it's better than what we used to have. :-)

@Varriount
Contributor
Varriount commented Jun 19, 2016 edited

Considering how new the module is, I think some incompatible changes can be made (if nothing else, this module can be deprecated and a new version made under a different name, such as 'rand'.

@ReneSac
Contributor
ReneSac commented Jun 19, 2016 edited

Araq: I disagree. Your comment made me remember that we had (and still have even if now the name clashes) Blaxpirit's random that is waaay better than the current random lib. It has none of the problems that I listed. Why wasn't it incorporated to nim libs instead of this new unrevised module? Too big?

@dom96
Member
dom96 commented Jun 19, 2016

The idea was to bring BlaXpirit's random lib into the standard library at some point. Moving the random proc into a random module was just the beginning.

@dom96
Member
dom96 commented Jun 19, 2016

But yes, the library is rather large and we need to review/edit it to ensure that it fits into the standard library. Not always an easy task unfortunately.

@ReneSac
Contributor
ReneSac commented Jun 19, 2016 edited

Blindly re-implement BlaXpirit's lib functionality wrong and with a different and broken interface is the begining of bringing it to the stdlib? I'm sorry for being harsh, but this don't seems a right step to make.

I don't see why this random module had to be created, as it only increases the difficulty of merging BlaXpirit work as far as I can see. It ends up there is a good reason why BlaXpirit lib don't offers a random[T](x: Slice[T]): T proc (it need branches to handle all the cases correctly, besides the name clash with the module of course), and it offers more directly what I needed (the would be random[T](low(T) .. high(T))).

EDIT: In BlaXpirit lib, randomInt(int.low, int.high) might be bad too and there is no warning. So it would also overflow or silently fail.

@ReneSac ReneSac referenced this issue in oprypin/nim-random Jun 19, 2016
Open

randomInt may fail for large ranges #11

@Araq
Member
Araq commented Jun 20, 2016

It doesn't blindly re-implement anything, it provides what math.nim used to provide.

@Araq Araq added a commit that closed this issue Aug 4, 2016
@Araq Araq fixes #4353 5d56361
@Araq Araq closed this in 5d56361 Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment