Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixing issue 7936 #542

Closed
wants to merge 2 commits into from

3 participants

@jkrempus

This fixes the issue 7936. When the user passed a random generator to randomSample, gen was assinged after prime() was first called, so I moved gen assignment into the RandomSample constructor.

std/random.d
((9 lines not shown))
- this(R input, size_t howMany, size_t total)
- {
- _input = input;
- _available = total;
- _toSelect = howMany;
- enforce(_toSelect <= _available);
- // we should skip some elements initially so we don't always
- // start with the first
- prime();
- }
+ private void init(R input, size_t howMany, size_t total)
@jmdavis Collaborator
jmdavis added a note

Never create a function named init. It probably shouldn't even be legal. It will cause problems with the type's init property.

Also, thish should just stay as a constructor. One constructor can call another. The new constructor would then be something like

this(R input, size_t howMany, size_t total, Random gen)
{
    this.gen = gen;
    this(input, throwMany, total);
}
@jkrempus
jkrempus added a note

I forgot about the init property, thanks. I've renamed init to initialize now. It should certainly not stay a constructor. When Random is not void, using such a constructor to construct a RandomSample instance would always result in a bug that this pull request fixes. If a function can not safely construct an instance of a type, it should not be a constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
std/random.d
((37 lines not shown))
+ }
+
+ this(R input, size_t howMany, size_t total)
+ {
+ init(input, howMany, total);
+ }
+ }
+ else
+ {
+ static if (hasLength!R)
+ this(R input, size_t howMany, Random _gen)
+ {
+ this(input, howMany, input.length, _gen);
+ }
+
+ this(R input, size_t howMany, size_t total, Random _gen)
@jmdavis Collaborator
jmdavis added a note

Please do not prepend _ to a variable name if it's not a member variable. You can just leave it as gen and do this.gen = gen;.

@jkrempus
jkrempus added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@WebDrake

I've uploaded an alternative fix as part of my pull request implementing the Vitter sampling algorithm. I think this provides a better solution as it also handles the case where the user doesn't pass a random number generator at all.

@jkrempus

Oh, I never thought about a problem with copying a RandomSample that uses the global rng. It looks like your fix is the only way to fix that. I'm closing this pull request.

@jkrempus jkrempus closed this
@WebDrake

My fault -- I didn't raise it in the bug report. I only noted the case where the sampler is passed a URNG with different seeds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 18, 2012
  1. @jkrempus

    Fixed bug 7936

    jkrempus authored
Commits on May 4, 2012
  1. @jkrempus
This page is out of date. Refresh to see the latest.
Showing with 31 additions and 10 deletions.
  1. +31 −10 std/random.d
View
41 std/random.d
@@ -1498,13 +1498,8 @@ struct RandomSample(R, Random = void)
/**
Constructor.
*/
- static if (hasLength!R)
- this(R input, size_t howMany)
- {
- this(input, howMany, input.length);
- }
- this(R input, size_t howMany, size_t total)
+ private void initialize(R input, size_t howMany, size_t total)
{
_input = input;
_available = total;
@@ -1515,6 +1510,34 @@ Constructor.
prime();
}
+ static if(is(Random == void))
+ {
+ static if (hasLength!R)
+ this(R input, size_t howMany)
+ {
+ this(input, howMany, input.length);
+ }
+
+ this(R input, size_t howMany, size_t total)
+ {
+ initialize(input, howMany, total);
+ }
+ }
+ else
+ {
+ static if (hasLength!R)
+ this(R input, size_t howMany, Random gen)
+ {
+ this(input, howMany, input.length, gen);
+ }
+
+ this(R input, size_t howMany, size_t total, Random gen)
+ {
+ this.gen = gen;
+ initialize(input, howMany, total);
+ }
+ }
+
/**
Range primitives.
*/
@@ -1608,8 +1631,7 @@ auto randomSample(R)(R r, size_t n) if (hasLength!R)
auto randomSample(R, Random)(R r, size_t n, size_t total, Random gen)
if(isInputRange!R && isUniformRNG!Random)
{
- auto ret = RandomSample!(R, Random)(r, n, total);
- ret.gen = gen;
+ auto ret = RandomSample!(R, Random)(r, n, total, gen);
return ret;
}
@@ -1617,8 +1639,7 @@ if(isInputRange!R && isUniformRNG!Random)
auto randomSample(R, Random)(R r, size_t n, Random gen)
if (isInputRange!R && hasLength!R && isUniformRNG!Random)
{
- auto ret = RandomSample!(R, Random)(r, n, r.length);
- ret.gen = gen;
+ auto ret = RandomSample!(R, Random)(r, n, r.length, gen);
return ret;
}
Something went wrong with that request. Please try again.