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