Hello.
Le mar. 29 mars 2022 à 17:08, Avijit Basak <avijit.ba...@gmail.com> a écrit :
>
> Hi All
>
> Please find my comments below.
>
> [...]
>
> --I have made the changes and created a new PR. Kindly review the same and
> share your thoughts.
> https://github.com/apache/commons-math/pull/208
I've merged PR #208 into the feature branch (please open a
new one for changes entailed by the comments below).
I again had to delete the branch (and recreate it with the merged
changes from PR #208). [I must be missing something about the
correct git workflow...]
There seems to be something wrong in the "examples-ga-tsp"
application (fitness does not change).
At the end of the run, one should be able to quickly assess the
goodness of the solution; the new code prints a line with many
"Node [...]" elements while the "--legacy" switch prints the "best"
fitness and a list of indices. In either case, the solution should
consist of the list of visited cities (one per line) and the total
distance.
I can't seem to find how the logger is configured. Currently, all
"INFO" messages are logged to the "standard error" console; one
should be able to e.g. redirect output to a file, or set the log level.
There is still a mix between library code and application code (but
this is to be discussed in MATH-1643.
>From browsing the library code, I'm tempted to believe that the
dependency towards a logging framework is not necessary (or
underused). I think that such a feature could be left to the application
layer (per the "ConvergenceListener" registry).
Likewise, the "PopulationStatisticsLogger" is not general enough to
be worth being part of the library.
A few (nit-pick) remarks about code style in general.
Javadoc is incomplete: All methods must be documented.
Please avoid redundant links like e.g.
---CUT---
/**
* @param crossoverPolicy The {@link CrossoverPolicy}
* @param mutationPolicy The {@link MutationPolicy}
* @param selectionPolicy The {@link SelectionPolicy}
* @param convergenceListeners An optional collection of
* {@link ConvergenceListener} with
variable arity
*/
@SafeVarargs
protected AbstractGeneticAlgorithm(final CrossoverPolicy<P> crossoverPolicy,
final MutationPolicy<P> mutationPolicy,
final SelectionPolicy<P> selectionPolicy,
ConvergenceListener<P>... convergenceListeners) {
this.crossoverPolicy = crossoverPolicy;
this.mutationPolicy = mutationPolicy;
this.selectionPolicy = selectionPolicy;
updateListenerRigistry(convergenceListeners);
}
---CUT---
Readers of the HTML-generated doc can already click on the various
arguments within the signature; so there is no need to add visual noise
in the source code just to be able to click from within the Javadoc part
just above that signature.
The Javadoc block above should be
---CUT---
/**
* @param crossoverPolicy Crossover policy.
* @param mutationPolicy Mutation policy.
* @param selectionPolicy Selection policy.
* @param convergenceListeners Collection of user-defined listeners.
*/
---CUT---
[Note the absence of "The" and the presence of a final "period".]
A blank line is welcome to separate ideas ("logical" blocks of code)
However, there should not be an empty line after a closing brace if
it is followed by another closing brace.
Also, in all recent codes, there is no blank line between the instance
fields; the (mandatory) Javadoc is enough to logically (and visually)
separate the fields.
In "AbstractGeneticAlgorithm":
* There should be a single constructor (handling default values should
be left to the application layer). [This would allow the removal of
"updateListenerRigistry" method (note: There is a typo in that name).]
* Are annotations (@SafeVarargs, ...) necessary? Please document.
In "AdaptiveGeneticAlgorithm":
* There should be a single constructor (same remark as above).
* Why the use of reflection ("isAssignableFrom")?
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
