commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex D Herbert (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (RNG-96) AhrensDieterMarsagliaTsangGammaSampler incorrectly names parameters
Date Thu, 25 Apr 2019 12:23:00 GMT

    [ https://issues.apache.org/jira/browse/RNG-96?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16826007#comment-16826007
] 

Alex D Herbert commented on RNG-96:
-----------------------------------

Yes it is a naming mismatch similar to MATH-1373.

The MathWorld link labels the parameters alpha and theta. Their definitions of skewness and
kurtosis use the alpha parameter.

The Wikipedia link labels the parameters k or alpha as shape and theta as scale. With these
definitions the skewness and kurtosis also use the k or alpha parameter.

So MathWorld and Wikipedia match their definitions.

The variance uses both parameters so can be used as a test:

{noformat}
variance = alpha * theta^2
{noformat}
 
A simple test shows that the distribution is wrong according to this:

{code:java}
@Test
public void testVariance() {
    checkVariance(1, 2);
}

private void checkVariance(double alpha, double theta) {
    final RestorableUniformRandomProvider rng =
        RandomSource.create(RandomSource.SPLIT_MIX_64);
    final AhrensDieterMarsagliaTsangGammaSampler sampler =
        new AhrensDieterMarsagliaTsangGammaSampler(rng, alpha, theta);
    final SummaryStatistics stats = new SummaryStatistics();
    for (int i = 0; i < 10000; i++) {
        stats.addValue(sampler.sample());
    }
    // From https://en.wikipedia.org/wiki/Gamma_distribution
    final double expected = alpha * theta * theta;
    Assert.assertEquals(expected, stats.getVariance(), expected * 0.1);
}
{code}

This fails but when alpha and theta are reversed for the constructor it passes.

So should I do a PR that just updates the names and Javadoc and maintain the usage of the
input parameters?

Or a more breaking change to maintain the parameter order and update the class internals to
correctly use the input parameters as named? This would break any code of a user who is currently
running the sampler with a known workaround of swapping the parameters and did not think to
raise a bug. But it will fix the code of any user who read the javadoc, passed the parameters
as stated and is getting incorrect samples.



> AhrensDieterMarsagliaTsangGammaSampler incorrectly names parameters
> -------------------------------------------------------------------
>
>                 Key: RNG-96
>                 URL: https://issues.apache.org/jira/browse/RNG-96
>             Project: Commons RNG
>          Issue Type: Bug
>          Components: sampling
>    Affects Versions: 1.3
>            Reporter: Alex D Herbert
>            Assignee: Alex D Herbert
>            Priority: Minor
>             Fix For: 1.3
>
>
> The AhrensDieterMarsagliaTsangGammaSampler has parameters alpha and theta.
> Using the naming conventions on [Wikipedia Gamma distribution|https://en.wikipedia.org/wiki/Gamma_distribution]
the alpha parameter is also known as the shape and the theta parameter is the scale:
> {noformat}
> // Wikipedia
> alpha = shape
> theta = scale
> {noformat}
> However if the sampler is run with the same parameters as the Wikipedia article histograms
of the output sample distribution does not match. They need to be swapped indicating a naming
mismatch.
> Studying the same algorithm in o.a.c.math3.distribution.GammaDistribution it appears
that the theta parameter is being used by Commons RNG as the shape and the alpha parameter
is being used as the scale. So the names are incorrect and have been swapped.
> {noformat}
> // Commons RNG
> alpha = scale (wrong)
> theta = shape (wrong)
> {noformat}
> The unit tests for this sampler does this:
> {code:java}
> // Gamma (theta < 1).
> add(LIST, new org.apache.commons.math3.distribution.GammaDistribution(unusedRng,
>     thetaGammaSmallerThanOne, alphaGamma),
>     new AhrensDieterMarsagliaTsangGammaSampler(
>             RandomSource.create(RandomSource.XOR_SHIFT_1024_S),
>             alphaGamma, thetaGammaSmallerThanOne));
> // Gamma (theta > 1).
> add(LIST, new org.apache.commons.math3.distribution.GammaDistribution(unusedRng, 
>     thetaGammaLargerThanOne, alphaGamma),
>     new AhrensDieterMarsagliaTsangGammaSampler(
>               RandomSource.create(RandomSource.WELL_44497_B),
>                                                
>               alphaGamma, thetaGammaLargerThanOne));
> {code}
> This is a different parameter order for the two samplers.
>  So the tests enforce the fact that the parameters are swapped between Commons Math3
and Commons RNG.
> Changing the actual parameter order would be a change of functionality. So this can be
fixed by updating the Javadoc and parameter names for the sampler.
> {noformat}
> // Commons RNG
> alpha renamed to theta (scale)
> theta renamed to alpha (shape)
> {noformat}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message