[jira] [Commented] (RNG-176) Enhance the UniformRandomProvider interface with extra methods and default implementations

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

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

Alex Herbert commented on RNG-176:
----------------------------------

I have created [PR 111|https://github.com/apache/commons-rng/pull/111] with enhancements to the UniformRandomProvider interface. This adds default methods for all uniform sampling and stream methods found in the JDK 17 RandomGenerator interface. It adds default implementations for all existing methods except nextLong. The interface is thus a functional interface and implementing a RNG requires providing a source of random bits as a long:
{code:java}
UniformRandomProvider rng = new SecureRandom()::nextLong;
{code}
The addition of default implementations to existing interface methods triggered an error in JApiCmp. I have tested the changes with:
 * [JApiCmp|https://siom79.github.io/japicmp/]
 * [JAPI Compliance Checker|https://lvc.github.io/japi-compliance-checker/]
 * [revapi|https://revapi.org/revapi-site/main/index.html]

Here is what is flagged as binary incompatible by each using their default settings across the current 1.5-SNAPSHOT:
||Problem||JApiCmp||JAPI||revapi||
|Add default method to interface|x *| | |
|Change interface method to default |x| | |
|Remove class method|x|x| |
|Add abstract method to enum| | |x|
h3. Add default method to interface

This is adding a new default method to an interface. It is one of the reasons that default methods were introduced: to allow enhancement of the JDK collections API without breaking binary compatibility. Here the settings for JApiCmp are wrong. The other tools allow this change.

* There is a fix for this in Commons Parent to change the behaviour to allow adding default methods.
h3. Change interface method to default

This is adding a default implementation for an existing interface method. JApiCmp again flags this is binary incompatible. The other tools do not. JAPI notes this in its report as a change with no effect. revapi has an entry for this in the documentation that states the effect is equivalent (see [java.method.nowDefault|https://revapi.org/revapi-java/0.26.1/differences.html#java.method.nowDefault]).
h3. Remove class method

This is removing the redundant implementations of UniformRandomProvider from the commons-rng-core classes BaseProvider, IntProvider and LongProvider. Since the methods have the same implementation in the interface these could be removed. This is marked as binary incompatible by JApiCmp and JAPI. revapi detects this as allowed. Since JAPI uses javap to decompile classes in the package jar files it does not create a full class hierarchy for the core module, i.e. it does not know about the default methods in the client interface. I assume that japicmp may also not fully consider the dependencies.

I compiled a project against Commons RNG 1.4. Then updated the 1.5-SNAPSHOT to remove the redundant methods. My projects entire test suit (with lots of random number usage) ran without issue. However I did have to explicitly include the 1.5-SNAPSHOT of commons-rng-core. My project POM only includes the sampling and simple modules. Without explicit inclusion of the correct core and client jars I had some run time errors due to linkage issues with missing methods.

This issue will effect projects with large dependencies trees that include versions via dependencies. As such it may be better to leave the current methods in the abstract base providers in the core module. This results in some small duplication in the codebase but will prevent any linkage errors from a mismatched set of commons RNG modules.
h3. Add abstract method to enum

This was also flagged during testing. A new abstract method has been added in commons-rng-simple NativeSeedType that is called by an existing method. Such a change would be breaking if it was an abstract class since all derived classes must have the method. However this is an enum. Thus the only derived classes are also declared within the same codebase. It is not possible for any external packages to extend the enum, therefore addition of an abstract method can be ignored as all possible enum instances of the class would have to provide the method to be able to compile the code.

This is in an {{internal}} package. As such the defaults for JAPI ignore this. If the settings are changed to include-internal then this is flagged as a minor issue.

It seems that both JAPI and revapi do not distinguish abstract methods in an enum as separate from abstract methods in a class. JApiCmp does not flag this as an issue (it would be [java.method.abstractMethodAdded|https://revapi.org/revapi-java/0.26.1/differences.html#java.method.abstractMethodAdded] for a class).

This highlights that there is not one tool that correctly identifies all changes in the current 1.5-SNAPSHOT. This prevents changing from using japicmp to revapi without adding custom configuration to ignore non-breaking changes in an internal package. As such I added custom configuration to the japicmp maven plugin to allow interface methods to change to have a default implementation.

I have not yet added documentation to the user guide to explain similarities and differences between UniformRandomProvider and JDK 17 RandomGenerator. This could be added a distinct section, e.g. titled 'Commons RNG vs java.util.random (JDK 17)'. This allows the section to cover:
 * UniformRandomProvider vs RandomGenerator
 * JumpableUniformRandomProvider vs JumpableGenerator
 * LongJumpableUniformRandomProvider vs LeapableGenerator
 * X vs StreamableGenerator
 * X vs SplittableGenerator
 * X vs ArbitrarilyJumpableGenerator

Some of the interfaces have no equivalents. Addition of streaming functionality may be possible using default methods added to the current interfaces.

See also [Javadoc java.util.random|https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/package-summary.html]

 

> Enhance the UniformRandomProvider interface with extra methods and default implementations
> ------------------------------------------------------------------------------------------
>
>                 Key: RNG-176
>                 URL: https://issues.apache.org/jira/browse/RNG-176
>             Project: Commons RNG
>          Issue Type: New Feature
>    Affects Versions: 1.4
>            Reporter: Alex Herbert
>            Assignee: Alex Herbert
>            Priority: Major
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> JDK 17 introduced the {{RandomGenerator}} interface with the following methods:
> {code:java}
> DoubleStream doubles();
> DoubleStream doubles(double randomNumberOrigin, double randomNumberBound);
> DoubleStream doubles(long streamSize);
> DoubleStream doubles(long streamSize, double randomNumberOrigin,
>                      double randomNumberBound);
> IntStream ints();
> IntStream ints(int randomNumberOrigin, int randomNumberBound);
> IntStream ints(long streamSize);
> IntStream ints(long streamSize, int randomNumberOrigin,
>                int randomNumberBound);
> LongStream longs();
> LongStream longs(long randomNumberOrigin, long randomNumberBound);
> LongStream longs(long streamSize);
> LongStream longs(long streamSize, long randomNumberOrigin,
>                  long randomNumberBound);
> boolean nextBoolean();
> void nextBytes(byte[] bytes);
> float nextFloat();
> float nextFloat(float bound);
> float nextFloat(float origin, float bound);
> double nextDouble();
> double nextDouble(double bound);
> double nextDouble(double origin, double bound);
> int nextInt();
> int nextInt(int bound);
> int nextInt(int origin, int bound);
> long nextLong();
> long nextLong(long bound);
> long nextLong(long origin, long bound);
> double nextGaussian();
> double nextGaussian(double mean, double stddev);
> double nextExponential();
> {code}
> The only method that is *non-default* is {{{}nextLong{}}}. This allows a new generator to be simply implemented by providing the source of randomness as 64-bit longs.
> The {{UniformRandomProvider}} interface can be expanded to include these generation methods. Using Java 8 default interface methods will not require any changes to generators currently implementing the interface.
> I propose to:
>  # Add the new methods for streams and numbers in a range.
>  # Add default implementations of the current API. These can be extracted from the  o.a.c.rng.core.BaseProvider implementations.
>  # Remove the implementations in o.a.c.rng.core.BaseProvider. This change would be binary compatible.
> The base classes in commons core for 32-bit and 64-bit sources of randomness, IntProvider and LongProvider, can be updated suitably to only override the default interface methods where they can be more efficiently implemented given the source of randomness. This applies to:
> ||Source||Update||Details||
> |int|nextBytes|Use nextInt() for the source of bytes|
> | |nextBoolean|Use a cached int for the randomness|
> | |nextInt|Directly supply the int rather than using 32-bits from nextLong()|
> | |nextDouble|Optimise the bits used from two ints for the 53-bits required for the double.|
> |long|nextInt; nextBoolean|Use a cached long for the randomness|
> h3. Note 1
> The UniformRandomProvider also has the method:
> {code:java}
> void nextBytes(byte[] bytes,
>                int start,
>                int len);
> {code}
> This can also have a default implementation using the output from nextLong().
> h3. Note 2
> The methods to generate an exponential and Gaussian are already implemented in the {{commons-rng-sampling}} module.
> java.util.Random has a nextGaussian() method and so this method appears to be for backward compatibility with legacy Java code. The method is implemented using a modified Ziggurat sampler which uses an exponential sampler for the long tail. The API has thus exposed the exponential sampling method that is used internally in the nextGaussian implementation.
> With no backward compatibility requirements the Commons RNG interface can avoid the distribution sampling methods. Users should select an appropriate sampler from the sampling module.
>  



--
This message was sent by Atlassian Jira
(v8.20.7#820007)