Hello Samy. Le lun. 7 juin 2021 à 11:51, Samy Badjoudj <samy.badjo...@gmail.com> a écrit : > > Hello Gilles, > > Is there something we need to change > https://github.com/apache/commons-math/pull/190, that one comes after > the discussion we had.
I probably failed to convey it but the main point of that discussion was that we need more info about the subject (and use-cases too) in order to be confident about what the API should be (or not). The two of us agreeing that the current design can be improved is a necessary condition but not sufficient to ensure that e.g. your PR is a long-term improvement: I'd be wary to add a new package to the "legacy" module as that would just mean that we know that more work is needed. I remind that a goal of the modularization is to create a non-"legacy" module with each functionality that can stand on its own. The low-discrepancy classes are obvious candidates. [They depend on the legacy exception classes but that can be trivially fixed. The hard part it to come up with real use-cases to drive the discussion on the design.] It's great if you dig further into this. But it's also great if you take on more pragmatic issues like those that could block the release of v4.0. [Cf. other thread.] Thanks, Gilles > Thanks > > On 01.06.21 20:42, Samy Badjoudj wrote: > > > > On 01.06.21 20:02, Gilles Sadowski wrote: > >> Le mar. 1 juin 2021 à 15:25, Samy Badjoudj <samy.badjo...@gmail.com> > >> a écrit : > >>> Thanks Gilles, please find below my remarks > >>> > >>> > >>> On 01.06.21 12:37, Gilles Sadowski wrote: > >>>> Le lun. 31 mai 2021 à 02:07, Gilles Sadowski <gillese...@gmail.com> > >>>> a écrit : > >>>>> Hello. > >>>>> > >>>>> Halton and Sobol sequences have been implemented in the "random" > >>>>> package. From Wikipedia[1]: > >>>>> ---CUT--- > >>>>> Low-discrepancy sequences are also called quasirandom sequences, > >>>>> due to their common use as a replacement of uniformly distributed > >>>>> random numbers. The "quasi" modifier is used to denote more clearly > >>>>> that the values of a low-discrepancy sequence are neither random nor > >>>>> pseudorandom. > >>>>> ---CUT--- > >>>>> > >>>>> TL;DR; > >>>>> I propose to create an interface "LowDiscrepancySequence" to properly > >>>>> represents the concept (as opposed to "RandomVectorGenerator"). > >>>> In current "master", interface "RandomVectorGenerator" is not used > >>>> anymore; I'll remove it. > >>>> > >>>> The new (untested) API would be: > >>>> ---CUT--- > >>>> public interface LowDiscrepancySequence extends Supplier<double[]> { > >>>> /** > >>>> * Creates a copy of the LowDiscrepancySequence and then > >>>> advances > >>> I would do first the advance then the copy it. > >> Then you'd end up with two instances having the same state... > > +1, right, it reminds me JumpableUniformRandomProvider ;) > >>>> /* the state of the current instance. The copy is returned. > >>>> * > >>>> * @param jump How far to jump from the current state. > >>>> * @return a copy of this instance. > >>>> */ > >>>> LowDiscrepancySequence jump(int jump); > >>>> } > >>>> > >>>> public class SobolSequence implements LowDiscrepancySequence { /* > >>>> ... */ } > >>>> > >>>> public class HaltonSequence implements LowDiscrepancySequence { /* > >>>> ... */ } > >>>> ---CUT--- > >>>> > >>>> TBD (through reading about the subject): Is this interface common to > >>>> all such algorithms? [Is "jump" always defined? Should its argument > >>>> rather be a "long"?] > >>> The maximum size of an array (underlying object) is limited by the max > >>> integer value. I would suggest keeping an it > >> Which "underlying" array are you referring to? > >> How would it impact the size of the "jump"? > > After looking at the Sobol and Halton sequences plus doing some > > testing, we can go for a long > >>>> From an usage POV, I don't see the purpose of the "skipTo" and > >>>> "getNextIndex" methods. > >>> Agree > >>>> By the way, Javadoc for "skipTo" mandates that the arg be positive, > >>>> but no check is performed; and the code produces a result (expected > >>>> or not?). > >>> It will return 0 vector in HaltonSequenceGenerator since > >> A bug then. > > Right I did check this condition > >>> for (int i =0; i <dimension; i++) { > >>> int index =count; double f =1.0 /base[i]; int j =0; while > >>> (index >0) { > >>> final int digit = scramble(i, j, base[i], index %base[i]); > >>> v[i] += f * digit; index /=base[i]; // floor( index / base ) f > >>> /=base[i]; } > >>> } > >>> > >>> For the SobolSequence the Gray Code bit manipulation is working just > >>> for unsigned number so unexpected value behavior. > >> A comparison should be done with another implementation. > >> Maybe the range can be extended to all "int" values (?). > >> > >>> Meaning we can do a sanity check in the jump method before. > >> +1 > >> > >>>> Another improvement for "SobolGenerator" would be to move the IO > >>>> to a factory/builder class (that would also cache the data it reads > >>>> from > >>>> the "resource" file). > >>>> > >>> That will be a good idea as a separate new improvement > >>> > >>> > >>>>> [1] https://en.wikipedia.org/wiki/Low-discrepancy_sequence > >>>> > >>>> > >>> /** Interface to Low Discrepancy Sequence generator and supplier * > >>> Supplier of a low discrepancy vectors * Offers navigation through > >>> underlying sequence */ public interface > >>> LowDiscrepancySequenceextends Supplier<double[]> { > >>> /** * Skip to the index in the sequence * @param index of the > >>> element to > >>> skip to * @return T element at the index */ > >>> LowDiscrepancySequencejump(int index); ----> return a copy of it to > >>> avoid side effect } > >>> > >>> > >>> > >>> > >>> /** * private constructor avoid side effects * @return copy of > >>> LowDiscrepancySequence */ private LowDiscrepancySequencecopy() { > >>> return new SobolSequenceGenerator(this); } > >>> > >>> > >>> /** * private constructor avoid side effects * @return copy of > >>> LowDiscrepancySequence */ private LowDiscrepancySequencecopy() { > >>> return new HaltonSequenceGenerator(this); } > >>> > >> Another line of improvement would be to mimic "RandomSource": > >> https://gitbox.apache.org/repos/asf?p=commons-rng.git;a=blob;f=commons-rng-simple/src/main/java/org/apache/commons/rng/simple/RandomSource.java;h=65ca02fdc285fbb7ea8305008dbce21f571191d7;hb=HEAD > > > > Thanks for the link I will have a closer look, never checked the rng > > repo btw :) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org