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
