Fixing issue 7936 #542

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@jkrempus
Contributor

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.

@jmdavis jmdavis and 1 other commented on an outdated diff May 4, 2012
- 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
jmdavis May 4, 2012 Member

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 May 4, 2012 Contributor

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.

@jmdavis jmdavis and 1 other commented on an outdated diff May 4, 2012
+ }
+
+ 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
jmdavis May 4, 2012 Member

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 May 4, 2012 Contributor

Fixed.

@WebDrake
Contributor
WebDrake commented May 9, 2012

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
Contributor
jkrempus commented May 9, 2012

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 May 9, 2012
@WebDrake
Contributor
WebDrake commented May 9, 2012

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