[rng] UnitSphereSampler factory constructor

[Alex Herbert <al...@gmail.com>]

When updating the documentation for new samplers I noticed that the shape
samplers accepted the RNG as the last argument to the factory constructor
methods:

double[] a = {1, 2}
double[] b = {3, 4}
UniformRandomProvider rng = RandonSouce.KISS.create()
BoxSampler sampler = BoxSampler.of(a, b, rng);

All the distribution samplers take the RNG as the first argument. The other
utility samplers for collections and combinations/permutations also take
the RNG as the first argument.

The shape samplers were based on the UnitSphereSampler which takes the RNG
as the last argument. It appears to be an odd-one-out.

I have updated the shape samplers to take the RNG as the first argument:

BoxSampler sampler = BoxSampler.of(rng, a, b);

The constructor for the UnitSphereSampler cannot be changed due to BC. But
the factory constructor is currently unreleased. This could be changed to
accept the RNG as the first argument. It would make upgrading code more
difficult to achieve with a simple regex:

int dimension = 3;
UnitSphereSampler s1 = new UnitSphereSampler(dimension, rng);
UnitSphereSampler s2 = UnitSphereSampler.of(rng, dimension);

IMO we should set the RNG as the first argument to the factory constructor
for consistency across the entire package. I would also mark the public
constructor as deprecated. The public constructor creates a wrapper class
around a delegate which implements sampling. Use of the factory constructor
directly creates the optimal sampler. So use of the constructor should be
avoided. Marking it as deprecated would encourage users to change to the
optimal factory constructor.

Any opinions on this?

Re: [rng] UnitSphereSampler factory constructor

[Gilles Sadowski <gi...@gmail.com>]

Le ven. 23 juil. 2021 à 23:23, Alex Herbert <al...@gmail.com> a écrit :
>
> When updating the documentation for new samplers I noticed that the shape
> samplers accepted the RNG as the last argument to the factory constructor
> methods:
>
> double[] a = {1, 2}
> double[] b = {3, 4}
> UniformRandomProvider rng = RandonSouce.KISS.create()
> BoxSampler sampler = BoxSampler.of(a, b, rng);
>
> All the distribution samplers take the RNG as the first argument. The other
> utility samplers for collections and combinations/permutations also take
> the RNG as the first argument.
>
> The shape samplers were based on the UnitSphereSampler which takes the RNG
> as the last argument. It appears to be an odd-one-out.
>
> I have updated the shape samplers to take the RNG as the first argument:
>
> BoxSampler sampler = BoxSampler.of(rng, a, b);
>
> The constructor for the UnitSphereSampler cannot be changed due to BC. But
> the factory constructor is currently unreleased. This could be changed to
> accept the RNG as the first argument. It would make upgrading code more
> difficult to achieve with a simple regex:
>
> int dimension = 3;
> UnitSphereSampler s1 = new UnitSphereSampler(dimension, rng);
> UnitSphereSampler s2 = UnitSphereSampler.of(rng, dimension);
>
> IMO we should set the RNG as the first argument to the factory constructor
> for consistency across the entire package. I would also mark the public
> constructor as deprecated. The public constructor creates a wrapper class
> around a delegate which implements sampling. Use of the factory constructor
> directly creates the optimal sampler. So use of the constructor should be
> avoided. Marking it as deprecated would encourage users to change to the
> optimal factory constructor.
>
> Any opinions on this?

Agreed.

Gilles

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org