Hello.
Side-note: Please try to not remove quotes from previous emails,
as it leads to a confusing sequence (e.g. things I wrote previously
now appear below as quotes from your last message).
Le dim. 7 nov. 2021 à 10:34, Avijit Basak <avijit.ba...@gmail.com> a écrit :
>
> Hi All
>
> Please find my comments below:
>
>
> [...]
>
> > >
> > > (B)
> > > I'm confused by your defining "legacy" packages in new modules...
> > > What kind of comparisons are you considering?
> > > It is fine to depend (with scope "test") on CM v3.6.1 to perhaps (?)
> > > regression testing; but please note that when your proposal is
> > > merged, it will imply that the "legacy" codes *must* be removed.
> > > [We don't want to keep duplicate functionality.]
> > > -- The new implementation has improved the quality of optimization over
> > the
> > > legacy model.
> >
> > "Improved" in what sense?
> > If you mean enhanced performance, such checks should be done
> > using JMH (producing data to be published on the web site).
> >
> > --Along with performance and memory utilization, stochastic algorithms
> have
> > another comparison parameter "quality of result". In stochastic
> algorithms,
> > global optimum is not guaranteed, We have to compare the quality of the
> > result along with performance and memory consumption to compare two
> > algorithm implementations. I have kept the legacy example just for
> > comparison between the new and the legacy implementations.
>
> Great that you take care of checking improvements on the quality
> measures. Just make sure that the new code do not depend on
> anything in module "commons-math-legacy". As noted earlier, a
> dependency on a previous release of CM with
> <scope>test</scope>
> is fine.
>
> --test scope can only be used for test packages. However, this is kept in
> src of example module. Changing previous release's dependency scope to test
> is showing compilation error in src package. I am not sure how to manage
> this.
Depending on released code (e.g. version 3.6.1 of Commons Math)
is fine too for the "main" codes of the "examples" module.
> >
> > Also (if no objections are made):
> > 1. the declaration should go in the main POM (in the
> <dependencyManagement>
> > section).
> > 2. the library (CM) must not depend on a specific logger implementation.
> > --Do you mean I should also remove the simple implementation
> "slf4j-simple"
> > in commons-math-ga.
>
> Yes.
> Specific implementations a user's choice that could be different in
> each of the "examples" modules.
>
> --Done.
>
> > [...]
> > >
> > > * Class "RandomGenerator"
> > > 1. Duplicates functionality (storage of thread-local instances)
> > > readily available in "Commons RNG".
> > > 2. (IMHO) Thread-local instances should not be used for "heavy" usage
> > > (like in GA).
> > > --Modified
> >
> > You still use "ThreadLocalRandomSource".
> > I think that if some functionality requires a RNG instance it should be
> > passed to its constructor (or created by it).
> >
> > I noticed that the interface "ChromosomeRepresentationUtils"
> > is actually a utility class (as the name indicates): It only contains
> > "static" functions. This will probably require refactoring (utility
> > classes are frowned upon nowadays).
> > Some functionality in that class (e.g. sampling from a range of
> > integers) exists in the "sampling" module of "Commons RNG".
> >
> > --Will look into this.
>
> I understand that "hiding" the access to the RNG functionality makes
> it for a seemingly nicer API, but I'm not sure that we should do it given
> that the "random" aspect is a fundamental, and explicit, part of the GA
> algorithm.
> How about introducing "factories" to create the objects that use a RNG,
> giving them their "own" RNG instance. For example:
> ---CUT---
> public interface MutationPolicy<P> {
> Chromosome<P> mutate(Chromosome<P> original, double mutationRate);
>
> interface Factory<P> {
> /**
> * Creates an instance with a dedicated source of randomness.
> *
> * @param rng RNG algorithm.
> * @param seed Seed.
> * @return an instance that must <em>not</em> be shared among
> threads.
> */
> MutationPolicy<P> create(RandomSource rng, Object seed);
>
> default MutationPolicy<P> create(RandomSource rng) {
> return create(rng, null);
> }
> }
> }
> ---CUT---
> ?
>
> --[IMHO] We can give users the option of choosing RandomSource. However,
> choosing the RandomSource in all places separately seems to be little
> overwork for the users.
Setting up all the "policies" (mutation, crossover, ...) must be done
anyways, by the user; the above factories just add an argument to
be passed at instantiation.
> Separate RandomSource for each place may be
> redundant.
I'm not sure what you mean by "redundant".
Since the RNG instances are not thread-safe, either separate sources
*must* be used or the synchronization must be handled separately (as
done for the "ThreadLocalRandomSource" possibly with a performance
loss).
> I also believe the system should provide a default option for
> RandomSource instead of completely depending on the choice of users.
Why?
This is a library, and the RNG should be viewed as an input (user's choice
to be made at the application level).
> What
> if we provide the feature of configuring RandomSource in our
> RandomNumberGenerator(earlier RandomGenerator) class like this. Please
> share your thoughts regarding this.
>
> --CUT--
> /** The default RandomSource for random number generation. **/
> private static RandomSource randomSource =
> RandomSource.XO_RO_SHI_RO_128_PP;
>
> /**
> * Sets the random source for this random generator.
> * @param randomSource
> */
> public static void configure(RandomSource randomSource) {
> RandomNumberGenerator.randomSource = randomSource;
> }
>
> /**
> * constructs the singleton instance.
> */
> private RandomNumberGenerator() {
> }
>
> /**
> * Returns the (static) random generator.
> * @return the static random generator shared by GA implementation
> classes
> */
> public static UniformRandomProvider getRandomGenerator() {
> return
> ThreadLocalRandomSource.current(RandomNumberGenerator.randomSource);
> }
> --CUT--
There are multiple problems (already noted previously):
1. It hardcodes a specific "default" source (whereas the GA functionality
is "agnostic" about which source is actually used).
2. The RNG instance is shared among all the classes that need it (which
makes access unnecessarily slower).
3. The static field "randomSource" is mutable; this is looking for trouble
(race condition).
> [...]
>
>>>> [...]
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
