Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reallow float ranges in random module #11199

Merged
merged 1 commit into from
May 8, 2019
Merged

Reallow float ranges in random module #11199

merged 1 commit into from
May 8, 2019

Conversation

mratsim
Copy link
Collaborator

@mratsim mratsim commented May 8, 2019

Fix #11198 while preserving #10998 fix for #7698

@krux02
Copy link
Contributor

krux02 commented May 8, 2019

And here the tail of pull requests of regarding float ranges begins. I see 19 more issues and pull requests related to float ranges.

Anyway, maybe you should mention in a comment that for float ranges the upper bound is exclusive.

result = T(rand(r, int(x.b) - int(x.a)) + int(x.a))

proc rand*[T: Ordinal](x: HSlice[T, T]): T =
let f = r.rand(-1.0 .. 1.0)
Copy link
Contributor

@kaushalmodi kaushalmodi May 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mratsim While you are working on this PR, can you add the spaces around dotdot in the above 3 doAsserts too?

PS: I am wondering how the doAsserts on rands always pass .. How is it predicted that the first two rands will always give 4, and the third will always give 6?! Seems like one or more of those doasserts is bound to fail some time.

@mratsim
Copy link
Collaborator Author

mratsim commented May 8, 2019

It's not float range as in range[0.0..1.0] but float slices HSlice[float, float] that were disabled though.

@krux02
Copy link
Contributor

krux02 commented May 8, 2019

I always get confused about ranges and slices. In my opinion they should be ranamed to range and RangeType or something. It will open up the Identifier Slice for other use cases (I could use it). But that is another topic.

Try this in your code and tell me what you think about it:

import random

var a : 0 .. 10

var bins: array[0..10, int]

for i in 0 .. 100:
  let x = rand(a.type)
  bins[x] += 1

echo bins

bins = default(typeof(bins))

var b : 0.0 .. 10.0

for i in 0 .. 100:
  let x = rand(b.type)
  bins[int(floor(x))] += 1

echo bins

I can't test it right now, but I know what will happen. I think you can't do anything about it, but I think this pitfall is something that could be mentioned int he documentation.

@mratsim
Copy link
Collaborator Author

mratsim commented May 8, 2019

Array ranges are transformed into slices by type but range types are not

binning.nim(18, 15) Error: type mismatch: got <type range 0.0..10.0(float64)>
but expected one of: 
proc rand(max: int): int
proc rand[T: Ordinal or SomeFloat](x: HSlice[T, T]): T
proc rand(r: var Rand; max: Natural): int
proc rand[T](r: var Rand; a: openArray[T]): T
proc rand(max: float): float
proc rand[T: SomeInteger](t: typedesc[T]): T
proc rand(r: var Rand; max: range[0.0 .. high(float)]): float
proc rand[T](a: openArray[T]): T
proc rand[T: Ordinal or SomeFloat](r: var Rand; x: HSlice[T, T]): T

expression: rand(type(b))

Reworked:

import random, math

var a : 0 .. 10

var bins: array[0..10, int]

for i in 0 .. 100:
  let x = rand(a.type)
  bins[x] += 1

echo "int:", bins

bins = default(typeof(bins))

var b : 0.0 .. 10.0

for i in 0 .. 100:
  let x = rand(0.0 .. 10.0)
  bins[int(floor(x))] += 1

echo "float:", bins

Result

int:[6, 14, 8, 7, 4, 18, 5, 11, 10, 8, 10]
float:[12, 9, 8, 10, 13, 10, 11, 3, 16, 9, 0]

So yeah the 11th bin is empty but it's the integer ranges being inclusive that is unusual and ..< is good enough for me. Other languages struggle when inclusive integer bounds are needed. I never needed inclusive floats though.

@Araq Araq added the merge_when_passes_CI mergeable once green label May 8, 2019
@Araq Araq merged commit 65b6250 into nim-lang:devel May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Floating point random module regression
4 participants