[
https://issues.apache.org/jira/browse/RNG-57?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16639999#comment-16639999
]
Alex D Herbert commented on RNG-57:
-----------------------------------
{quote}Please review the changes made for RNG-58.
{quote}
Nice modifications. I noticed that the {{BaseProvider}} has the signature:
{code:java}
protected byte[] composeStateInternal(byte[] state,
byte[] parentState) {
{code}
but the javadoc order for {{parentState}} and {{state}} are reversed. So I
assume you originally wrote it the other way around then swapped them. With no
state from the parent it wouldn't be detected in a test.
Suffice to say it doesn't work as is and I had to swap them back to:
{code:java}
protected byte[] composeStateInternal(byte[] parentState,
byte[] state) {
{code}
This is the simplest change to get things working. However the order in the
combined state is state+parentState so logically the arguments should be state
and parentState. But all the sub-classes do this:
{code:java}
return composeStateInternal(super.getStateInternal(),
state);
{code}
So parent should be first. It is also documented to work this way.
So I've just added some comments in the code that the order is state+parent
when combining and splitting.
The test for {{ProvidersCommonParametricTest.testStateSettable}} passes. So the
combining and splitting of the state is working.
I am now getting some failures in {{ProvidersCommonParametricTest}} due to
randomness.
The failures are always the same. Each RNG is built using a fixed seed and I
assume JUnit is running the tests in the same order.
However the changes to cache calls from {{next}} (either {{int}} or {{long}})
means that the sequence that is tested after the first call to {{nextBoolean}}
for an {{IntProvider}}, or the first call to {{nextBoolean/nextInt}} for an
{{LongProvider}} is different. These are the failures:
{noformat}
[ERROR] Failures:
[ERROR]
ProvidersCommonParametricTest.testUniformNextIntegerInRange:165->checkNextIntegerInRange:400->checkNextIntegerInRange:420->checkNextInRange:552
org.apache.commons.rng.core.source32.MultiplyWithCarry256: Too many failures
for n = 1834691456 (11 out of 500 tests failed)
[ERROR]
ProvidersCommonParametricTest.testUniformNextLongInRange:180->checkNextLongInRange:438->checkNextInRange:552
org.apache.commons.rng.core.source64.XorShift1024Star: Too many failures for n
= 4611686018427387902 (11 out of 500 tests failed)
{noformat}
- MultiplyWithCarry256 is an {{IntProvider}} and so the method {{next(int)}}
is unchanged. The failures are due to the state of the sequence being shifted
after a call of nextBoolean.
- XorShift1024Star is a {{LongProvider}} and the method {{next(int)}} has been
changed. The failures may actually be due to the loss of randomness from using
the using the upper and lower bits from the long as separate {{int}} samples.
However both tests only fail with 11/500 (p=0.022) which is 1 failure above the
critical p-value of 0.02.
I've pushed to the same PR branch so you can view the changes.
Q. What to do to fix this?
- Change the fixed seed for each of the failing providers until it works.
- Change the critical p-value.
> CachedUniformRandomProvider for nextBoolean() and nextInt()
> -----------------------------------------------------------
>
> Key: RNG-57
> URL: https://issues.apache.org/jira/browse/RNG-57
> Project: Commons RNG
> Issue Type: Improvement
> Components: sampling
> Affects Versions: 1.2
> Reporter: Alex D Herbert
> Priority: Minor
> Labels: performance
>
> Implement a wrapper around a {{UniformRandomProvider}} that can cache the
> underlying source of random bytes for use in the methods {{nextBoolean()}}
> and {{nextInt()}} (in the case of {{LongProvider}}). E.g.
> {code:java}
> LongProvider provider = RandomSource.create(RandomSource.SPLIT_MIX_64);
> CachedLongProvider rng = new CachedLongProvider(provider);
> // Uses cached nextLong() 64 times
> rng.nextBoolean();
> // Uses cached nextLong() twice
> rng.nextInt();
> IntProvider provider = RandomSource.create(RandomSource.KISS);
> CachedIntProvider rng2 = new CachedIntProvider(provider);
> // Uses cached nextInt() 32 times
> rng2.nextBoolean();
> // This could be wrapped by a factory method:
> UniformRandomProvider rng = CachedUniformRandomProviderFactory.wrap(
> // Any supported source: IntProvider or LongProvider
> RandomSource.create(RandomSource...));
> {code}
> The implementation should be speed tested to determine the benefit for
> {{nextBoolean()}} and if {{nextInt()}} can be improved for {{LongProviders}}.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
