[jira] Commented: (MATH-310) Supply nextSample for all distributions with inverse cdf using inverse transform sampling approach

["Phil Steitz (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MATH-310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12842500#action_12842500 ] 

Phil Steitz commented on MATH-310:
----------------------------------

See discussion here: http://markmail.org/message/kolivuytbt5cj25s

As stated on the mailing list, I am +0/1 on the idea of adding generic inversion-based generators that work with any invertible distribution; but I still do not see attaching them to the distribution implementations as a good idea. This is for three reasons: 0) I see it as poor separation of concerns (admittedly this is a matter of taste, but I do not see sourcing random deviates as an essential behavior of a probability distribution) 1) if the implementation is *only* inversion-based, it will be naive for some distributions and we do not want users to get a bad impl by default 2) to fix 1) we have to essentially refactor our package structure to place random data generation into the distributions package, causing users to have to instantiate distributions and also configure generators to get deviates. I see it as simpler and more natural to use a RandomData instance. I am -1 on dropping the random package. Therefore, I am not in favor of attaching this functionality to the distributions.   

I propose that we resolve this issue by 

1) Extend RandomDataImpl to include deviate generation from all currently implemented distributions, using generic inversion as defined below where this is the best available method, or where we have not yet implemented a better one.  In 3.0, we can either eliminate the RandomData interface or extend it to include the new methods.

2) Add a generic nextInversionDeviate(distribution, generator) method similar to that defined in the patch to RandomDataImpl.  Doc the fact that for distributions having a nextXxx implementation, this should be used in place of nextInversionDeviate unless inversion is specifically desired.



> Supply nextSample for all distributions with inverse cdf using inverse transform sampling approach
> --------------------------------------------------------------------------------------------------
>
>                 Key: MATH-310
>                 URL: https://issues.apache.org/jira/browse/MATH-310
>             Project: Commons Math
>          Issue Type: Improvement
>    Affects Versions: 2.0
>            Reporter: Mikkel Meyer Andersen
>            Priority: Minor
>         Attachments: patch_proposal
>
>   Original Estimate: 3h
>  Remaining Estimate: 3h
>
> To be able to generate samples from the supported probability distributions, a generic function nextSample is implemented in AbstractContinuousDistribution and AbstractIntegerDistribution. This also gives the possibility to override the method if better algorithms are available for specific distributions as shown in the small example with the exponential distribution.
> Because the nextExponential is used several places: in nextPoisson it can be replaces by an instance if the ExponentialDistribution and in ValueServer it can as well, although maybe not in as natural maner as the other. 
> This problem with the Exponential is a special problem. In general the nextSample-approaches immediately gives the possibility the sample from all the distributions with inverse cdf instead just only a couple.
> Only AbstractContinuousDistribution and AbstractIntegerDistribution extends AbstractDistribution, and both AbstractIntegerDistribution and AbstractContinuousDistribution has an inverseCumulativeProbability-function. But in AbstractContinuousDistribution the inverse cdf returns a double, and at AbstractIntegerDistribution it - naturally - returns an integer. Therefor the nextSample is not put on AbstractDistribution, but on each extension with different return types.
> RandomGenerator as parameter instead of getting a RNG inside the nextSample, because one typically wants to use the same RNG because often several random samples are wanted. Another option is to have a RNG as a field in the class, but that would be more ugly and also result in several RNGs at runtime.
> The nextPoisson etc. ought to be moved as well, if the enhancement is accepted, but it should be a quick fix.
> Tests has to be written for this change as well.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.