Hi All
Please find my comments below:
>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...]
-- I have taken the latest code from apache repo.
>There seems to be something wrong in the "examples-ga-tsp"
>application (fitness does not change).
-- I have fixed the issue in fitness function calculation.
>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 have made the changes.
>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 a dependency for "slf4j-simple" in the "examples-ga" module
which would print all log messages in the console. There was a discussion
earlier on this and the decision was to use console for example modules for
log messages. Library users would add the implementation of slf4j according
to their need and provide required configurations to control the log levels
and messages.
>
>There is still a mix between library code and application code (but
>this is to be discussed in MATH-1643.
-- Kindly update the JIRA with your suggestions.
>From browsing the library code, I'm tempted to believe that the
>dependency towards a logging framework is not necessary (or
>underused).
-- Do you mean dependency on the slf4j?
I think that such a feature could be left to the application
>layer (per the "ConvergenceListener" registry).
-- ConvergenceListener is only to log the nature of convergence,
not internal details of all operators. Currently we have a very low number
of log messages which can be increased later.
>Likewise, the "PopulationStatisticsLogger" is not general enough to
>be worth being part of the library.
-- I would love to keep this simple implementation of convergence listener.
This can provide a very basic overview on the nature of convergence.
>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".]
>
-- Are we following this standard in the commons-math project? This would
initiate changes in almost all classes.
>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.
-- I have rectified this.
>
>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).]
--This is corrected.
>* Are annotations (@SafeVarargs, ...) necessary? Please document.
-- This annotation is necessary for any parameterized vararg. This is also
used in legacy classes like o.a.c.m.l.a.i.FieldHermiteInterpolator and
o.a.c.m.l.o.n.RungeKuttaFieldStepInterpolator.
>In "AdaptiveGeneticAlgorithm":
>* There should be a single constructor (same remark as above).
-- Removed the constructor with default argument.
>* Why the use of reflection ("isAssignableFrom")?
-- Replaced it by instanceof.
-- Created a new PR https://github.com/apache/commons-math/pull/209.
Thanks & Regards
--Avijit Basak
On Sun, 3 Apr 2022 at 19:52, Gilles Sadowski <gillese...@gmail.com> wrote:
> 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
>
>
