Hi All
Thanks for the review comments. Please find my comments below.
(1)
>A commit log message should strive to be informative
>for the reviewer; saying the like of "fixed minor bugs" does
>not convey anything.
>Even minor changes, like e.g. formatting cleanup, should be
>designated as such.
>For this PR, the message (which I've amended) was misleading
>because the change was not about bugs, but about removing
>GUI code (and its dependency).
--I have maintained a detailed commit message this time.
(2)
>The "GeneticException" class seems to mostly deal with "illegal"
>arguments; hence it should be a subclass of the JDK's standard
>"IllegalArgumentException" (and be renamed accordingly).
>If other condition types are needed, then another internal class
>should be defined with the corresponding standard semantics.
--IMHO if we think of a single exception class we should extend it only
from RuntimeException. If we think of multiple exception classes in one
module we may need to think of a base exception class. Other classes would
extend the same. The approach mentioned above would mix up these two.
Please share your opinion regarding this.
>[Exception messages need review for spelling and formatting.]
-- It will be really helpful if you can point out some specific examples.
(3)
>IMO Javadoc should avoid redundant phrases like "This class" as
>the first words of a class description.
--Refractored the javadoc comments. Please review and mention if you need
any further changes.
>A similar remark holds for fields in "GeneticException" class: Since
>the name of the field is self-documenting, duplication in the Javadoc
>is visual noise ("Message template" is concise and clear enough).
--Removal of the javadoc comments produces a checkstyle error.
>Similarly, simple accessors don't need the exact same sentence
>repeated twice (a single "@return ..." tag is sufficient).
--Modified.
(4)
>Class "ConvergenceListenerRegistry<P>" is generic but its code
>contains undocumented "@SuppressWarnings" annotations.
>Moreover, it is a singleton, and not thread-safe.
>Why should there be such a global "registry"?
>Since it is only accessed by the "AbstractGeneticAlgorithm" class,
>it could be defined as a private inner class.
--Made it a private inner class.
(5)
>In class "AbstractGeneticAlgorithm", methods "getCrossoverPolicy"
>"getMutationPolicy", "getElitismRate" are public, yet they are only
>ever called by a subclass.
--Modified the public to protected.
(6)
>Why support inheritance for "AbstractGeneticAlgorithm"?
>Why would users need their own subclass, rather than call those
>implemented within the library (currently, "GeneticAlgorithm" and
>"AdaptiveGeneticAlgorithm")?
>Couldn't we encapsulate the choice of algorithm in an "enum",
>similar to "RandomSource" in [RNG].
>Do I understand correctly that the (only?) difference between the
>two classes is the ability to adapt crossover and mutation rates?
-- The difference between GeneticAlgorithm and AdaptiveGeneticAlgorithm is
the ability to adapt crossover and mutation probability. However, as per
my understanding enum encapsulation is appropriate with the same set and
type of constructor arguments, where the arguments can be provided during
enum declaration. In our case the arguments would be provided by the client
program and cannot be pre-initialized as part of an enum declaration.
(7)
>The currently available GA implementations are sequential.
>IIUC, the "nextGeneration" methods should provide an option
>that processes the population using multiple threads.
--This needs to be done. However, I would like to address this along with
parallel GA i.e. convergence of multiple populations together.
(8)
>Do not use explicit "\n" and "\r" characters.[1]
--Done
Thanks & Regards
--Avijit Basak
On Mon, 7 Feb 2022 at 07:57, Gilles Sadowski <gillese...@gmail.com> wrote:
> Hello.
>
> A few remarks (as of PR #205) and questions:
>
> (1)
> A commit log message should strive to be informative
> for the reviewer; saying the like of "fixed minor bugs" does
> not convey anything.
> Even minor changes, like e.g. formatting cleanup, should be
> designated as such.
> For this PR, the message (which I've amended) was misleading
> because the change was not about bugs, but about removing
> GUI code (and its dependency).
>
> (2)
> The "GeneticException" class seems to mostly deal with "illegal"
> arguments; hence it should be a subclass of the JDK's standard
> "IllegalArgumentException" (and be renamed accordingly).
> If other condition types are needed, then another internal class
> should be defined with the corresponding standard semantics.
> [Exception messages need review for spelling and formatting.]
>
> (3)
> IMO Javadoc should avoid redundant phrases like "This class" as
> the first words of a class description.
> A similar remark holds for fields in "GeneticException" class: Since
> the name of the field is self-documenting, duplication in the Javadoc
> is visual noise ("Message template" is concise and clear enough).
> Similarly, simple accessors don't need the exact same sentence
> repeated twice (a single "@return ..." tag is sufficient).
>
> (4)
> Class "ConvergenceListenerRegistry<P>" is generic but its code
> contains undocumented "@SuppressWarnings" annotations.
> Moreover, it is a singleton, and not thread-safe.
> Why should there be such a global "registry"?
> Since it is only accessed by the "AbstractGeneticAlgorithm" class,
> it could be defined as a private inner class.
>
> (5)
> In class "AbstractGeneticAlgorithm", methods "getCrossoverPolicy"
> "getMutationPolicy", "getElitismRate" are public, yet they are only
> ever called by a subclass.
>
> (6)
> Why support inheritance for "AbstractGeneticAlgorithm"?
> Why would users need their own subclass, rather than call those
> implemented within the library (currently, "GeneticAlgorithm" and
> "AdaptiveGeneticAlgorithm")?
> Couldn't we encapsulate the choice of algorithm in an "enum",
> similar to "RandomSource" in [RNG].
> Do I understand correctly that the (only?) difference between the
> two classes is the ability to adapt crossover and mutation rates?
>
> (7)
> The currently available GA implementations are sequential.
> IIUC, the "nextGeneration" methods should provide an option
> that processes the population using multiple threads.
>
> (8)
> Do not use explicit "\n" and "\r" characters.[1]
>
>
> Regards,
> Gilles
>
> [1] See
> https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#lineSeparator--
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
--
Avijit Basak
