Hi. Le mer. 13 févr. 2019 à 21:21, Alex Herbert <alex.d.herb...@gmail.com> a écrit : > > > > > On 13 Feb 2019, at 17:52, Gilles Sadowski <gillese...@gmail.com> wrote: > > > >> Le mer. 13 févr. 2019 à 18:47, Alex Herbert <alex.d.herb...@gmail.com> a > >> écrit : > >> > >>> On 13/02/2019 17:35, Gilles Sadowski wrote: > >>> Hello. > >>> > >>>> Le mer. 13 févr. 2019 à 17:20, <aherb...@apache.org> a écrit : > >>>> This is an automated email from the ASF dual-hosted git repository. > >>>> > >>>> aherbert pushed a commit to branch master > >>>> in repository https://gitbox.apache.org/repos/asf/commons-rng.git > >>>> > >>>> commit 367f022a88dedf2b79778f18ad1ed3ec655fffe8 > >>>> Author: aherbert <aherb...@apache.org> > >>>> AuthorDate: Wed Feb 13 16:20:21 2019 +0000 > >>>> > >>>> The commons-math distributions can use a null random generator > >>> Not a good move, I 'd think: this constructor has disappeared from > >>> the development version of CM.[1] > >> > >> The constructor is in the unit tests. These depend on commons-math3 in > >> the test scope. > >> > >> If the tests are upgraded to commons-math4 then this will just be a > >> compile error. The tests would have to be changed anyway to reference > >> the updated component package name for maths. > > > > I know; the point is that this constructor was a bad design > > decision (there are old and long discussions about it). > > So, why present wrong usage in [RNG] (that would a.o. lead > > to wondering why the constructor was deleted)? > > > > I thought I was changing to the recommended usage. > > The javadoc for the distributions in CM acknowledge that a generator is > created by default unless you supply one, and that it is used for sampling. > Given the tests are not sampling then null is recommended. > > The Junit test code can be updated with a better comment to reflect the > recommended usage. Currently it states: “The commons-math distributions are > not used for sampling so use a null random generator”. I can change to the CM > javadoc of: “In case no sampling is needed for the created distribution, it > is advised to pass null as random generator via the appropriate constructors > to avoid the additional initialisation overhead.” > > I can rename the RandomGenerator to ‘unusedRng’. Anyone who reads the code > should not be mislead by this.
I'm sorry for the perpetuation of the misunderstanding. The CM classes were wrong in initializing a RNG instance even though it might never be used. Hence the new API there[1] passes the RNG only when the user wants to sample. Passing null to the (deprecated) constructor is a workaround to avoid the unnecessary creation, as you note but it thus perpetuates bad usage; in the case of a unit test, documenting proper use is IMO more important than having a slight performance improvement. Note that if/when we release [Statisitics], the classes from there (where the API has been corrected) will be used in the test. HTH, Gilles [1] http://commons.apache.org/proper/commons-math/apidocs/src-html/org/apache/commons/math4/distribution/NormalDistribution.html#line.240 > > > Regards, > > Gilles > > > >> > >> Alex > >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
