[jira] [Commented] (RNG-142) Return type of method "withUniformRandomProvider"

["Alex Herbert (Jira)" <ji...@apache.org>]

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

Alex Herbert commented on RNG-142:
----------------------------------

The {{NormalizedGaussianSampler}} does not implement the {{SharedStateContinuousSampler}}. This is the issue:
{code:java}
ZigguratNormalizedGaussianSampler n01 = ZigguratNormalizedGaussianSampler.of(RandomSource.create(RandomSource.KISS));

GaussianSampler g = GaussianSampler.of(n01.withUniformRandomProvider(RandomSource.create(RandomSource.JSF_64)), 0.43, 2.1);
{code}
as {{ZigguratNormalizedGaussianSampler}} returns {{SharedStateContinuousSampler}} from {{withUniformRandomProvider}}.

All the classes in the {{distribution}} package return {{SharedStateContinuousSampler}}/{{SharedStateDiscreteSampler}} from {{withUniformRandomProvider}}. Many could be changed to return the type of the implementing class. However some use a delegate of a different class to do the sampling. This is due to adaption of legacy samplers such as the PoissonSampler and ChengBetaSampler which return samplers that do not extend from the parent class, mainly to avoid extending the deprecated SamplerBase class. A clean implementation would have the base class as abstract and then the implementations can extend from this. The abstract class can specify to return its own type and the returned class will be a sub-class. See the AliasMethodDiscreteSampler for an example where a sub-class specialisation can be used.

This has a problem in that both return types have advantages. It is nice to return the same type as the class that declared to implement the interface. It is also an advantage to be able to return any class that provides the functionality of the sampler. This allows choosing the implementation which may be different depending on the parameters.

The interfaces could have the return type of the method defined using a generic:
{code:java}
// Unchanged
public interface SharedStateSampler<R> {
    R withUniformRandomProvider(UniformRandomProvider rng);
}

// Add <R>
public interface SharedStateContinuousSampler<R>
    extends ContinuousSampler, SharedStateSampler<R> {
    // Composite interface
}

// Add <R>
public interface SharedStateDiscreteSampler<R>
    extends DiscreteSampler, SharedStateSampler<R> {
    // Composite interface
}

// Add <R>
public interface SharedStateObjectSampler<R extends SharedStateObjectSampler<R, T>, T> extends
    ObjectSampler<T>, SharedStateSampler<R> {
    // Composite interface
}
{code}
The SharedStateObjectSampler interface has not yet been released so changes here are fine. It would change samplers like this:
{code:java}
public class CollectionSampler<T> implements SharedStateObjectSampler<T> {

// to

public class CollectionSampler<T> implements SharedStateObjectSampler<CollectionSampler<T>, T> {
{code}
For the distributions the change requires adding a type, for example:
{code:java}
public class ZigguratNormalizedGaussianSampler
        implements NormalizedGaussianSampler, SharedStateContinuousSampler<ZigguratNormalizedGaussianSampler> {
    public ZigguratNormalizedGaussianSampler withUniformRandomProvider(UniformRandomProvider rng);
    public static ZigguratNormalizedGaussianSampler of(UniformRandomProvider rng);
}
{code}
I don't think it would break binary compatibility as the return type has changed to a sub-class. But I would have to go through all the code to make the changes and check. There may be some classes which are a problem.

 

> Return type of method "withUniformRandomProvider"
> -------------------------------------------------
>
>                 Key: RNG-142
>                 URL: https://issues.apache.org/jira/browse/RNG-142
>             Project: Commons RNG
>          Issue Type: Improvement
>          Components: sampling
>            Reporter: Gilles Sadowski
>            Priority: Major
>
> Expected usage:
> {code:java}
> NormalizedGaussianSampler n01 = ZigguratNormalizedGaussianSampler.of(RandomSource.create(RandomSource.KISS));
> GaussianSampler g = GaussianSampler.of(n01.withUniformRandomProvider(RandomSource.create(RandomSource.JSF_64)), 0.43, 2.1);
> {code}
> Code doesn't compile: Method {{withUniformRandomProvider}} returns a {{SharedStateContinuousSampler}} whereas a {{NormalizedGaussianSampler}} is required.
> Am I missing something?



--
This message was sent by Atlassian Jira
(v8.3.4#803005)