Hello.
Le lun. 14 févr. 2022 à 08:03, Avijit Basak <avijit.ba...@gmail.com> a écrit :
>
> Hi All
>
> Thanks for the review comments. Please find my comments below.
>
> (1)
> [...]
>
> (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.
"single exception class" is not a requirement (it cannot be since
we agreed some time ago that it was better to align with the JDK's
delineation of error conditions (IAE, NPE, ILSE, AE, ...).
> 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.
Please no. We'd taken that approach in "Commons Math" (cf.
base class now in module "commons-math-legacy-exception"),
as I've mentioned already IIRC: It was a failed experiment IMO.
[For more details, please refer to the archive of the "dev" ML.]
> The approach mentioned above would mix up these two.
> Please share your opinion regarding this.
Eventually, all new components ([RNG], [Number], [Geometry], ...)
adopted the simple approach of non-public API (ideally private
or package-private) exception classes only for the developer's
use (and the purpose of which is limited to avoiding duplication).
>
> >[Exception messages need review for spelling and formatting.]
> -- It will be really helpful if you can point out some specific examples.
We can fix this when the PR has reached some stability.
>
> (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.
I've not looked yet, but thanks for taking it into account.
Similarly to the previous point, these clean-ups can happen later.
> >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.
I did not suggest to remove any Javadoc, only to rephrase it as:
---CUT---
/** "Message template". */
---CUT---
> [...]
>
> (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.
Thanks.
[We should nevertheless address the other issues mentioned in
the above paragraph.]
>
> (5)
> >In class "AbstractGeneticAlgorithm", methods "getCrossoverPolicy"
> >"getMutationPolicy", "getElitismRate" are public, yet they are only
> >ever called by a subclass.
> --Modified the public to protected.
Thanks.
I nevertheless suspect (as hinted at while mentioning the
expected multi-thread feature) that there is a design issue
here. [I may be wrong, so I'll try to make my point stronger
after a more in-depth review.]
>
> (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.
Why not?
A constant rate seems like a (trivial) type of adaptive rate.
>
> (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.
The two features (multi-thread vs multiple populations) should
be implemented independently: Users that only need the "basic"
GA should also be able to take advantage of their machine's
multiple CPUs.
[This is related to the design issue which I mentioned previously.]
>
> (8)
> >Do not use explicit "\n" and "\r" characters.[1]
> --Done
Thanks,
Gilles
>> [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
