You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Alex Bertram (Created) (JIRA)" <ji...@apache.org> on 2012/03/07 10:08:58 UTC
[jira] [Created] (MATH-764) New sample() API should accept
RandomGenerator as parameter
New sample() API should accept RandomGenerator as parameter
-----------------------------------------------------------
Key: MATH-764
URL: https://issues.apache.org/jira/browse/MATH-764
Project: Commons Math
Issue Type: Improvement
Affects Versions: 3.0
Reporter: Alex Bertram
This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
That is:
interface RealDistribution {
Sampler createSampler(RandomGenerator generator);
Sample createSampler(); // uses default RandomGenerator
}
interface Sampler {
double sample();
double[] sample(int sampleSize);
}
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Dennis Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13285685#comment-13285685 ]
Dennis Hendriks commented on MATH-764:
--------------------------------------
bq. I would personally be OK with refactoring to do this move, ensuring that distributions (or at least the sampling methods) had configurable RandomGenerators.
Once again, the distributions already have random generators. But an alternative to having the random generator supplied via the constructor could be to remove the randomData field from the distributions altogether, and instead provide the random generator as argument to the sample method.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Comment Edited] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Dennis Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13284641#comment-13284641 ]
Dennis Hendriks edited comment on MATH-764 at 5/29/12 7:14 AM:
---------------------------------------------------------------
bq. [...] I would suggest, as Gilles mentions above, just using RandomDataImpl directly, which provides direct support for sampling with configurable RandomGenerator. The setup in the patch looks like a long way around the barn to just get back to what is there already in RandomDataImpl.
In my opinion, RandomDataImpl should just provide random data, and should not implement all the different distributions, as that is what the distributions (classes) themselves are for. It seems we currently use a mixed approach: some distributions are implemented as methods on the RandomDataImpl class (nextPoisson, nextExponential, nextUniform, nextBeta, ...), while other distributions (TriangularDistribution, ...) don't have a corresponding method in the RandomDataImpl class, and exist solely as distribution class. The methods in RandomDataImpl mostly seem to just use 'return nextInversionDeviate(new SomeDistributionClass(param1, param2, ..., paramn));' as method implementation. As such, we get a tight coupling between the distributions and the RandomDataImpl class. Would it not be better to separate the RandomData(Impl) from the distributions? Also, using nextInversionDeviate is probably not what we want, as most distribution classes have a sample method that provides a direct implementation instead of the generic nextInversionDeviate...
was (Author: dhendriks):
bq [...] I would suggest, as Gilles mentions above, just using RandomDataImpl directly, which provides direct support for sampling with configurable RandomGenerator. The setup in the patch looks like a long way around the barn to just get back to what is there already in RandomDataImpl.
In my opinion, RandomDataImpl should just provide random data, and should not implement all the different distributions, as that is what the distributions (classes) themselves are for. It seems we currently use a mixed approach: some distributions are implemented as methods on the RandomDataImpl class (nextPoisson, nextExponential, nextUniform, nextBeta, ...), while other distributions (TriangularDistribution, ...) don't have a corresponding method in the RandomDataImpl class, and exist solely as distribution class. The methods in RandomDataImpl mostly seem to just use 'return nextInversionDeviate(new SomeDistributionClass(param1, param2, ..., paramn));' as method implementation. As such, we get a tight coupling between the distributions and the RandomDataImpl class. Would it not be better to separate the RandomData(Impl) from the distributions? Also, using nextInversionDeviate is probably not what we want, as most distribution classes have a sample method that provides a direct implementation instead of the generic nextInversionDeviate...
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13285665#comment-13285665 ]
Phil Steitz commented on MATH-764:
----------------------------------
I am starting to think that my historical resistance to moving the sampling functionality entirely to within the distributions was wrong. I would personally be OK with refactoring to do this move, ensuring that distributions (or at least the sampling methods) had configurable RandomGenerators.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Dennis Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13284641#comment-13284641 ]
Dennis Hendriks commented on MATH-764:
--------------------------------------
bq [...] I would suggest, as Gilles mentions above, just using RandomDataImpl directly, which provides direct support for sampling with configurable RandomGenerator. The setup in the patch looks like a long way around the barn to just get back to what is there already in RandomDataImpl.
In my opinion, RandomDataImpl should just provide random data, and should not implement all the different distributions, as that is what the distributions (classes) themselves are for. It seems we currently use a mixed approach: some distributions are implemented as methods on the RandomDataImpl class (nextPoisson, nextExponential, nextUniform, nextBeta, ...), while other distributions (TriangularDistribution, ...) don't have a corresponding method in the RandomDataImpl class, and exist solely as distribution class. The methods in RandomDataImpl mostly seem to just use 'return nextInversionDeviate(new SomeDistributionClass(param1, param2, ..., paramn));' as method implementation. As such, we get a tight coupling between the distributions and the RandomDataImpl class. Would it not be better to separate the RandomData(Impl) from the distributions? Also, using nextInversionDeviate is probably not what we want, as most distribution classes have a sample method that provides a direct implementation instead of the generic nextInversionDeviate...
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13284066#comment-13284066 ]
Gilles commented on MATH-764:
-----------------------------
Sorry for the delay in coming back to this issue.
The patch must be missing quite a few important things, since it leads to 29 compilation errors.
Also, some of the changes certainly cannot make it in 3.1 because they'd break compatibility (namely the removal of the "sample" methods from the distribution classes; those should be marked as deprecated).
If you want to submit another patch, please also check the code formatting (no tabs, 4-space indentation). Thanks.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13285244#comment-13285244 ]
Gilles commented on MATH-764:
-----------------------------
bq. In my opinion, RandomDataImpl should just provide random data, and should not implement all the different distributions, as that is what the distributions (classes) themselves are for. [...]
I agree.
bq. Would it not be better to allow passing in a RandomData instance via the constructors? [...] This way, the distributions are still immutable.
That looks like a good solution.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Gilles (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13224303#comment-13224303 ]
Gilles commented on MATH-764:
-----------------------------
* In the current design, the specific distribution subclasses would need to implement both "RealDistribution" and "Sampler".
Can you give an example of a class that would implement "Sampler" interface?
* The addition of the "sample" methods in the distribution classes were accepted, as syntactic sugar, on the premisses that users should be satisfied with the default RNG.
* As much as possible, we try to have immutable objects; in this view, creating (reinitializing) the RNG after construction is not an option.
* If you need to access (or change) the RNG, the original methods designed to offer sequences of pseudo-random numbers (with certain properties) are located in package "random" (in class "RandomDataImpl").
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Gilles resolved MATH-764.
-------------------------
Resolution: Fixed
Fix Version/s: 3.1
A solution was implemented in revision 1363604.
Please open a new issue if it does not fulfill your needs.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Assignee: Gilles
> Fix For: 3.1
>
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Dennis Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13284644#comment-13284644 ]
Dennis Hendriks commented on MATH-764:
--------------------------------------
bq. A aimple way to provide the requested functionality without breaking compatibility might be to add setRandomGenerator methods to AbstractXxxDistribution. This violates immutability, but the reseedRandomGenerator methods currently provided arguably do this already with essentially the same practical consequences.
Would it not be better to allow passing in a RandomData instance via the constructors? That way, instead of constructing a default RandomDataImpl with default random generator, the user can provide their own RandomData, which they constructed using their own random generator. This way, the distributions are still immutable.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Dennis Hendriks (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13285680#comment-13285680 ]
Dennis Hendriks commented on MATH-764:
--------------------------------------
bq. I think that adding a RandomData instance as additional state to the Distribution classes would really add a new "responsibility" to the Distribution classes that muddies the API.
Actually, they already have this. For instance, AbstractRealDistribution has this:
{code}
/** RandomData instance used to generate samples from the distribution. */
protected final RandomDataImpl randomData = new RandomDataImpl();
{code}
The only problem is that we can't control what randomData contains, as it is fixed in the declaration of the field. Providing it via a constructor would solve this.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13284112#comment-13284112 ]
Phil Steitz commented on MATH-764:
----------------------------------
A aimple way to provide the requested functionality without breaking compatibility might be to add setRandomGenerator methods to AbstractXxxDistribution. This violates immutability, but the reseedRandomGenerator methods currently provided arguably do this already with essentially the same practical consequences. To implement this, randomData would have to be made non-final and the setter would have to recreate it using the supplied generator.
If this is distasteful, I would suggest, as Gilles mentions above, just using RandomDataImpl directly, which provides direct support for sampling with configurable RandomGenerator. The setup in the patch looks like a long way around the barn to just get back to what is there already in RandomDataImpl.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13285630#comment-13285630 ]
Gilles commented on MATH-764:
-----------------------------
bq. [...] have a separate set of interfaces/classes that sample from distributions: e.g. InversionSampler which would accept a Distribution and RandomData arguments to the constructor. [...]
Given the separation between the "...Distribution" classes and the sampling functionality located in "RandomData", if we want to keep this "historical" design (not my preferred option, but others will veto a merge), I also think that a new "SamplerFactory" would be the best approach. However I'm not sure that it would currently have enough "handles" into the internals of the Distribution implementations in order to provide the required functionality.
Anyway, best to talk with some code in had, so let's wait for your patch...
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Alex Bertram (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alex Bertram updated MATH-764:
------------------------------
Attachment: sampler-refactor.diff
Extraction of the sample functionality from the Distribution interfaces. All tests pass.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Alex Bertram (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13225109#comment-13225109 ]
Alex Bertram commented on MATH-764:
-----------------------------------
Hi Gilles,
Specifying the RandomGenerator is pretty important for my use case: I am working on the renjin project, which is an implementation of the R language on the JVM that relies heavily on Commons Math. We need to be able to generate random values for specific distributions using the random number generator of the type and seed specified by the user.
What i'm proposing would actually promote immutability and functional-style programming: The specific distribution classes would *not* implement Sampler, themselves, they would return a functor that implements Sampler.
Here's a few examples
{code:java}
interface RealDistribution {
/**
* Factory method that returns a Sampler function object for this
* distribution and the given {@code RandomGenerator}
*/
RealSampler createSampler(RandomGenerator rng);
}
interface RealSampler {
double sample();
double[] sample(int sampleSize);
}
abstract class AbstractRealSampler implements RealSampler {
RandomDataImpl randomData;
AbstractRealSampler(RandomGenerator rng) {
super();
this.randomData = new RandomDataImpl(rng);
}
double[] sample(int sampleSize) {
// loop impl
}
void reseedRandomGenerator(long seed) {
randomData.reSeed();
}
}
class AbstractRealDistribution implements RealDistribution {
RealSampler createSampler(RandomGenerator rng) {
return new AbstractRealSampler(rng) {
public double sample() {
return randomData.nextInversionDeviate(AbstractRealDistribution.this);
}
};
}
final RealSampler createSampler() {
return createSampler(DefaultRandomGeneratorFactory.create());
}
}
class ExponentialDistributionImpl extends AbstractRealDistribution {
RealSampler createSampler(RandomGenerator rng) {
return new AbstractSampler(rng) {
double sample() {
return randomData.nextExponential();
}
}
}
}
{code}
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13285869#comment-13285869 ]
Gilles commented on MATH-764:
-----------------------------
bq. Once again, the distributions already have random generators.
We know that. But see my very first comment:
bq. The addition of the "sample" methods in the distribution classes were accepted, as syntactic sugar, on the premisses that _users should be satisfied with the default RNG._
The dichotomy was to avoid to entirely duplicate the RNG functionality (both in "RandomData" and in the various distribution classes).
But if Phil is now OK with relocating the various method of "RandomData" to their corresponding distribution, that would solve the problem in another way: Deprecate (then remove) RandomData and make the RNG an argument to constructors of the distribution classes, as you propose.
We could also keep RandomData, for backwards compatibility, but have the methods redirect the actual computation to the distribution classes.
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (MATH-764) New sample() API should accept
RandomGenerator as parameter
Posted by "Alex Bertram (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13285615#comment-13285615 ]
Alex Bertram commented on MATH-764:
-----------------------------------
I think since the sample() method is already part of the 3.0 API, it might be better to just leave it alone as a convenience method and have a separate set of interfaces/classes that sample from distributions: e.g. InversionSampler which would accept a Distribution and RandomData arguments to the constructor. I think that adding a RandomData instance as additional state to the Distribution classes would really add a new "responsibility" to the Distribution classes that muddies the API.
Basically for the Renjin project we need to solve two concrete problems:
1) Given an arbitrary Distribution and an arbitrary random number generator provided by the R developer, create a new Sampler object that can sample values from the distribution. This makes it possible to cleanly implement R's large number of distribution-related built-in functions.
2) Provide specialized Sampler objects for certain distributions in order to be consistent with the original C-R implementation. While InversionSampler would be used most of the time, we have ported some of the specialized sampling algorithms for some of the more exotic distributions and would like to hide them behind a Sampler interface. Hence the desire to cleanly separate the Distribution, RandomData, and Sampler classes: a given distribution can have multiple sampling implementations, which each need to be paramaterized by the random number generator.
We can of course add this hierarchy to Renjin core and leave commons math alone, but I really like the idea of pushing as much of the pure math stuff into Commons Math and keeping Renjin focused on the R-language interpretration.
I will draft and post a new patch shortly.
Alex
> New sample() API should accept RandomGenerator as parameter
> -----------------------------------------------------------
>
> Key: MATH-764
> URL: https://issues.apache.org/jira/browse/MATH-764
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 3.0
> Reporter: Alex Bertram
> Attachments: sampler-refactor.diff
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> This may come to late as I know the 3.0 release is nearing completion, but I had some concerns about the new sample() method on the math3 RealDistribution interface.
> Specifically, there doesn't seem to be a way to supply a random generator to the sampler. Perhaps it would be better to have a factory method on the RealDistribution interface that accepted a RandomGenerator and returns an instance of some new interface, Sampler, which contains the sample() methods.
> That is:
> interface RealDistribution {
> Sampler createSampler(RandomGenerator generator);
> Sample createSampler(); // uses default RandomGenerator
> }
> interface Sampler {
> double sample();
> double[] sample(int sampleSize);
> }
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira