One principle reason for SharedStateDiscreteSampler and SharedStateContinuousSampler is to simplify the current design approach for samplers that use internal delegates to provide optimised algorithms. However this creates an inefficient outer class that is just wrapping the implementation.
The idea was to simplify the implementation of the SharedStateSampler<R> interface for these Discrete/Continuous Samplers. However it has wider application to moving away from public constructors to factory methods. The SharedStateDiscreteSampler interface would allow the samplers package to move to a factory constructor model where samplers have no public constructor. This allows the factory method to choose the best implementation. This idea is recommended by Joshua Block in Effective Java (see [1]). For example: UniformRandomProvider rng = ...; double mean = ...; PoissonSampler sampler = new PoissonSampler(rng, mean); vs (with some potential names): SharedStateDiscreteSampler sampler = PoissonSampler.newSharedStateDiscreteSampler(rng, mean); SharedStateDiscreteSampler sampler = PoissonSampler.newInstance(rng, mean); SharedStateDiscreteSampler sampler = PoissonSampler.newSampler(rng, mean); SharedStateDiscreteSampler sampler = PoissonSampler.newPoissonSampler(rng, mean); SharedStateDiscreteSampler sampler = PoissonSampler.of(rng, mean); Currently there are some unreleased classes in v1.3 that use various construction methods with different approaches to fitting within the current design of returning an instance that supports DiscreteSampler and SharedStateSampler<R> and using specialised algorithms: AliasMethodDiscreteSampler MarsagliaTsangWangDiscreteSampler GuideTableDiscreteSampler GeometricSampler The PoissonSamplerCache could also benefit from this as it returns a DiscreteSampler even though the returned object is now also a SharedStateSampler<R>. I would advocate introducing these interfaces and switching to a factory method design for unreleased code. This has no binary compatibility issues. Current released code that would also benefit from this design are those with internal delegates: PoissonSampler AhrensDieterMarsagliaTsangGammaSampler LargeMeanPoissonSampler ChengBetaSampler (no delegate but it chooses an algorithm) An example of factory code would be double alpha; double theta SharedStateContinuousSampler sampler = AhrensDieterMarsagliaTsangGammaSampler.of(rng, alpha, theta); SharedStateContinuousSampler sampler = AhrensDieterMarsagliaTsangGammaSampler.newGammaSampler(rng, alpha, theta); Since the class name is so verbose in this case the 'of' method name does not appear out of place. Note that as the SharedStateSampler<R> interface is implemented by all (non-deprecated) distribution samplers a factory method could be added to every sampler. This would pave the way for removal of public constructors in a 2.0 release (whenever that happens). Overall the proposal is to: - Create SharedStateDiscreteSampler and SharedStateContinuousSampler - Simplify the implementation of SharedStateSampler<R> - Move to factory constructors for unreleased samplers - Add factory constructors to existing samplers to return optimised implementations Thoughts on this are welcomed. Alex [1] https://www.baeldung.com/java-constructors-vs-static-factory-methods On Fri, 19 Jul 2019 at 10:35, Alex Herbert <alex.d.herb...@gmail.com> wrote: > This interface has been added in v1.3: > > /** > * Applies to samplers that can share state between instances. Samplers > can be created with a > * new source of randomness that sample from the same state. > * > * @param <R> Type of the sampler. > * @since 1.3 > */ > public interface SharedStateSampler<R> { > /** > * Create a new instance of the sampler with the same underlying state > using the given > * uniform random provider as the source of randomness. > * > * @param rng Generator of uniformly distributed random numbers. > * @return the sampler > */ > R withUniformRandomProvider(UniformRandomProvider rng); > } > > All of the DiscreteSampler and ContinuousSampler classes have been updated > to implement SharedStateSampler. > > This is the equivalent of: > > /** > * Sampler that generates values of type {@code int} and can create new > instances to sample > * from the same state with a given source of randomness. > * > * @since 1.3 > */ > public interface SharedStateDiscreteSampler > extends DiscreteSampler, > SharedStateSampler<SharedStateDiscreteSampler> { > // Marker interface > } > > > If this combined marker interface is added to the library it would > simplify a lot of the code for samplers which have internal delegates to > avoid casting. With this interface they can hold a > SharedStateDiscreteSampler rather than a DiscreteSampler delegate and > directly use it to create new delegates. > > For example PoissonSampler code which wraps a specific implementation: > > /** > * @param rng Generator of uniformly distributed random numbers. > * @param source Source to copy. > */ > private PoissonSampler(UniformRandomProvider rng, > PoissonSampler source) { > super(null); > > if (source.poissonSamplerDelegate instanceof SmallMeanPoissonSampler) { > poissonSamplerDelegate = > > ((SmallMeanPoissonSampler)source.poissonSamplerDelegate).withUniformRandomProvider(rng); > } else { > poissonSamplerDelegate = > > ((LargeMeanPoissonSampler)source.poissonSamplerDelegate).withUniformRandomProvider(rng); > } > } > > Becomes: > > private PoissonSampler(UniformRandomProvider rng, > PoissonSampler source) { > super(null); > poissonSamplerDelegate = > source.poissonSamplerDelegate.withUniformRandomProvider(rng); > } > > > I propose to add: > > public interface SharedStateDiscreteSampler > extends DiscreteSampler, > SharedStateSampler<SharedStateDiscreteSampler> { > // Marker interface > } > > public interface SharedStateContinuousSampler > extends ContinuousSampler, > SharedStateSampler<SharedStateContinuousSampler> { > // Marker interface > } > > >
