> On 21 Sep 2019, at 19:40, Alex Herbert <alex.d.herb...@gmail.com> wrote: > > > >> On 21 Sep 2019, at 17:46, Gilles Sadowski <gillese...@gmail.com >> <mailto:gillese...@gmail.com>> wrote: >> >> Hello. >> >>>>>> [...] >>>>>> >>>>>>> >>>>>>> 2. Add a method BitSet<T> getSupport() to the RandomSource enum, where T >>>>>>> is an enum that can be expanded as more features are added. Initially is >>>>>>> would be: >>>>>> >>>>>> +1 >>>>>> With "EnumSet", I guess. >>>>> >>>>> Yes. >>>>> >>>>>> >>>>>>> >>>>>>> enum RandomSourceSupport { >>>>>>> /** The source supported the {@link JumpableUniformRandomProvider} >>>>>>> interface. */ >>>>>>> JUMPABLE, >>>>>>> /** The source supported the {@link >>>>>>> LongJumpableUniformRandomProvider} >>>>>>> interface. */ >>>>>>> LONG_JUMPABLE, >>>>>>> } >>>>>> >>>>>> +1 >>>>>> >>>>>>> >>>>>>> Currently the RandomSource only has a isNativeSeed(Object) method. So >>>>>>> adding methods does add API clutter. It would be possible to also add >>>>>>> getNativeSeedLength() method as this information (available in the >>>>>>> javadoc) is now available in the factory code that builds generators. >>>>>> >>>>>> +1 >>>>> >>>>> >>>>> So this would add to RandomSource: >>>>> >>>>> int getSeedByteLength() >>>>> EnumSet<RandomSourceSupport> getSupport() >>>> >>>> I think that the API should not specify the type of "Set". >>>> Also, the enum should be an inner class. Thus: >>>> >>>> public class RandomSource { >>>> // ... >>>> >>>> public enum Support { >>>> JUMPABLE, >>>> // etc. >>>> } >>>> >>>> public Set<Support> getSupport() { >>>> final Set<Support> s = new EnumSet<>(Support.class); >>>> >>>> // ... >>>> >>>> return s; >>>> } >>>> } >>> >>> On reflection I think the use of Set<Enum> is not the best choice. >>> Currently we are only wanting to support the question of if this is >>> possible: >>> >>> (JumpableUniformRandomProvider) RandomSource.create(…) >>> (LongJumpableUniformRandomProvider) RandomSource.create(…) >>> >>> The use of an enum is not a good fit here as any LongJumpable is also a >>> Jumpable due to the inheritance hierarchy. So the set of an enum that >>> specifies one or the other or both can be incorrectly used to be >>> LongJumpable but not Jumpable. >> >> I'm not sure I follow: "incorrectly used" by who? >> The library will return a consistent set (if it contains "LONG_JUMPABLE", >> it will also contain "JUMPABLE"). >> >>> >>> Plus it would require returning a copy of the set for each invocation to >>> get the supported functionality. >> >> Is that a problem? >> [Assuming this is setup/one time initialization of an RNG.] >> >>> And either storing the set per enum value or computing is dynamically which >>> is possible... >> >> Yes; an implementation detail. >> >>> >>> So perhaps a method that is the inverse of the Class::isAssignableFrom >>> method. Since the RandomSourceInternal knows the RNG class this can be >>> implemented as: >>> >>> /** >>> * Determines if the class represented by this random source is either the >>> same >>> * as, or is a subclass or subinterface of, the class or interface >>> represented >>> * by the specified {@code Class} parameter. It returns true if so; >>> otherwise it returns >>> * false. >>> * >>> * @param type the {@code Class} object to be checked >>> * @return the boolean value indicating whether the class of this random >>> source >>> * can be assigned to objects of the specified type >>> */ >>> public boolean isAssignableTo(Class<?> type) { >>> return type.isAssignableFrom(internalIdentifier.getRng()); >>> } >>> >>> Allowing: >>> >>> RandomSource source = ... >>> If (source.isAssignableTo(JumpableUniformRandomProvider.class) { >>> (JumpableUniformRandomProvider) RandomSource.create(source); >>> } else { >>> // fail ... >>> } >> >> Then, what have you gained over "instanceof"? >> Yes, it will avoid an instantiation; but the price seems to be an ugly API >> (IMHO). > > Agreed. It is not a very nice API. A bit raw. > >> >>> >>> This option is actually the easiest to implement and if more interfaces are >>> added later then nothing needs to be done since it is self-detecting. >> >> Yes, but then I'd keep that for the internal implementation of "support": >> >> public enum RandomSource { >> // ... >> public boolean isLongJumpable() { >> return isAssignableTo(LongJumpableUniformRandomProvider.class); >> } >> >> private boolean isAssignableTo(Class<?> type) { >> return type.isAssignableFrom(internalIdentifier.getRng()); >> } >> } >> >> Do you foresee many other "properties" that would warrant sparing >> a trivial method for each? > > The only other functionality I am aware of is: > > - to advance a given number of steps (skipping). This is something that is in > the standard c++ library implementations of many generators. It is easy to do > for LCG based generators but others also. For example it is offered by the > reference PCG generators. However I do not know why you would want to do > this. > - splittable. This we have been over before. It is functionality tied to the > Java 1.8 stream API and the requirement to spawn new generators for child > workers in a thread pool of unknown size (thus unknown number of descendent > children ruling out the use of jumpable). So maybe worth looking at when > moving to Java 1.8 (e.g. to mimic new Random().doubles() -> DoubleStream with > any RNG that is splittable). > > I am happy to go with adding the ‘is’ properties: >
> isLongJumpable() > isJumpable() I’ve put these methods into a PR for RNG-116 [1]. I’ve also added: public int getSeedByteSize() However I note that the RandomSource has factory methods to create int[] and long[] but not byte[]. Given the seed size is reported in bytes it would make sense to have: public byte[] createByteArray(int) In the seed factory methods. [1] https://github.com/apache/commons-rng/pull/67 <https://github.com/apache/commons-rng/pull/67> > > The future may have: > > isSkippable() > isSplittable() > > Alex > >> >> Gilles >> >>>> >>>>> >>>>> The first should return the native seed size in bytes. Currently the >>>>> javadoc >>>>> contains the native seed length. This is the length of the seed array or 1 >>>>> if the seed is a Long/Integer. >>>>> >>>>> I think having the seed size in bytes is more generic and it doesn’t >>>>> matter >>>>> if the seed is an array or not. So perhaps the javadoc should contain the >>>>> human readable array length (if relevant) but the code uses the generic >>>>> byte >>>>> length allowing: >>>> >>>> +1 >>>> >>>>> >>>>> SecureRandom sr = new SecureRandom(); >>>>> RandomSource source = RandomSource.MWC_256; >>>>> UniformRandomProvider rng = RandomSource.create(source, >>>>> sr.generateSeed(source.getSeedByteLength()); >>>>> >>>>> In the case where the seed is a Long/Integer I suggest dropping the >>>>> javadoc: >>>>> "Native seed size: 1” as it is redundant. >>>> >>>> +1 >>>> >>>> Regards, >>>> Gilles >>>> >>>>>> >>>>>>> >>>>>>> Any other suggestions? >>>>>>> >>>>>>> Alex >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> <mailto:dev-unsubscr...@commons.apache.org> >> For additional commands, e-mail: dev-h...@commons.apache.org >> <mailto:dev-h...@commons.apache.org>
