Hi
Thanks for the review comments. I have started making the changes.
However, I have some queries regarding some of comments as noted below:
(B)
I'm confused by your defining "legacy" packages in new modules...
--This is kept for comparison purposes between the legacy and the new
implementation of GA.
(D) General design
Class "ConsoleLogger"
-> We should not reinvent the wheel. We should consider whether logging
is necessary, and in the affirmative, depend on the de facto standard:
"slf4j".
-- I don't see any use of a logging framework in the math library. That is
the reason I introduced ConsoleLogger. If we introduce a logging framework
we won't need this class at all. I think we should include the logger in
the root(commons-math\) pom.xml file so that all modules should be able to
use this.
Class "Constants"
-> Any data should be declared where its purpose is obvious.
-- We can declare the constants where it belongs but this might introduce
duplicate constants across different classes and hence reduce reusability.
* Class "AbstractListChromosome" (and subclasses)
Didn't we conclude that this was a very wasteful implementation of the
"chromosome" concept?
-- I have some concerns regarding this. I am not much aware of any
discussion regarding this conclusion.
Chromosomes are always conceptualized as collections of allele/genes. So We
need a collection of the *genotypes* anyway. Here List has been used as a
collection.
We need an abstraction for representing the collection of Genotype. All
crossover and mutation operators are based on this abstraction. This
enabled reuse of crossover and mutation operators for all chromosome types
which extend the abstraction. I am not sure how to achieve this reusability
without an abstraction.
Any domain specific new chromosome implementation extending the
AbstractListChromosome class can reuse all crossover and mutation operators.
For our proposed improvement of BinaryChromosome we should be able to
extend the AbstractChromosome (*not* AbstractListChromosome) for the new
class and provide the dedicated crossover and mutation operators for the
corresponding Genotype. Without an *explicit* abstraction, management of
crossover and mutation operators would be difficult.
Please share further thoughts regarding this.
* Class "GeneticException"
1. Should not be public (or should be defined in an "internal" package"[1]).
2. If various types (that map to different JDK subclasses of
RuntimeException,
e.g. "IllegalArgumentException" vs "NullPointerException") are necessary,
there could be a factory for creating the appropriate instance.
However, for "null" checks, please use the JDK utilities[2].
-- As of now we are managing all exception types by single GeneticException
class. So there is no factory.
-- Using JDK utilities for NullPointer would repeat this code in all
places. Is it fine?
Objects.requireNonNull(object,
Message.format(GeneticException.NULL_ARGUMENT, args));
* Class "ConvergenceListenerRegistry"
Shouldn't it be thread-safe?
-- Yes. We need this to be thread-safe for parallel multi-population
parallel genetic algorithms.
Thanks & Regards
--Avijit Basak
On Mon, 18 Oct 2021 at 23:13, Gilles Sadowski <gillese...@gmail.com> wrote:
> Hello.
>
> Sorry for the delay in reviewing.
>
> Le lun. 18 oct. 2021 à 09:35, Avijit Basak <avijit.ba...@gmail.com> a
> écrit :
> >
> > Hi All
> >
> > I have created PR#197 as mentioned earlier. Kindly let me know if
> > there is any concern or comments.
> > I have created another *PR#199* consisting of the changes with
> > adaptive probability generations.
>
> Please find below my first remarks.
>
> (A)
> Please "rebase" on "master".
> Please "squash" intermediate commits: For a new feature, a single commit
> should exist (that corresponds to the JIRA report describing it).
>
> (B)
> I'm confused by your defining "legacy" packages in new modules...
>
> (C)
> File
> "commons-math-examples/examples-ga/src/main/resources/spotbugs/spotbugs-exclude-filter.xml"
> does not belong there.
>
> (D) General design
> Class "ConsoleLogger"
> -> We should not reinvent the wheel. We should consider whether logging
> is necessary, and in the affirmative, depend on the de facto standard:
> "slf4j".
>
> Class "Constants"
> -> Any data should be declared where its purpose is obvious.
>
> * 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).
>
> * Class "ValidationUtils"
> -> should not be public (or should be defined in an "internal" package).
>
> * Class "AbstractListChromosome" (and subclasses)
> Didn't we conclude that this was a very wasteful implementation of the
> "chromosome" concept?
>
> * Package "convergencecond"
> -> Should probably be renamed "convergence".
>
> * Class "GeneticException"
> 1. Should not be public (or should be defined in an "internal"
> package"[1]).
> 2. If various types (that map to different JDK subclasses of
> RuntimeException,
> e.g. "IllegalArgumentException" vs "NullPointerException") are necessary,
> there could be a factory for creating the appropriate instance.
> However, for "null" checks, please use the JDK utilities[2].
>
> * Class "ConvergenceListenerRegistry"
> Shouldn't it be thread-safe?
>
> (E) Unit tests
> * src/test
> 1. New tests should use Junit 5.
> 2. "Example usages" probably belong in the "examples-ga" module.
>
> (F) Code readability
> * Please write one argument per line.
> * Write one condition check per line.
> * Avoid comments with no added value (like "constructor" for a
> constructor).
> * Avoid "ASCII art" (see e.g. "OnePointCrossover"); a link[2] is often
> preferable.
> * Do no duplicate documentation (see e.g. "OnePointCrossover").
>
> (G)
> Some files contain "tab" characters (e.g. "pom.xml").
>
> Best regards,
> Gilles
>
> [1]
> https://gitbox.apache.org/repos/asf?p=commons-math.git;a=tree;f=commons-math-neuralnet/src/main/java/org/apache/commons/math4/neuralnet/internal;h=c0bc1409cdee0932c973fae05f03e8288034c005;hb=HEAD
> [2]
> https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-
> [3] https://en.wikipedia.org/wiki/Crossover_(genetic_algorithm)
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
--
Avijit Basak
