Hi All
Please find my comments.
>> (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).
>
-- I shall make the changes. What could be the name for the Exception.
GeneticIllegalArgumentException would be fine?
>>
>> >[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---
>
-- Just for confirmation. Do you need the comment to be changed from
---CUT---
/** Error message for "out of range" condition. */
---CUT---
*to*
---CUT---
/** "out of range" */
---CUT---
[...]
>> (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.
-- Our algorithm classes accept various user provided arguments like
crossover, mutation and selection operators. Enum declaration requires all
of the arguments to be provided at the declaration time in the class itself
like
---CUT---
public enum RandomSource {
JDK(*ProviderBuilder.RandomSourceInternal.JDK*),
...
}
---CUT---
I am not sure how to achieve this for our algorithm classes.
>
>>
>> (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.]
>
-- I am thinking to leverage user's multiple CPUs for doing
multi-population GA. It would a global approach where same thread pool
would be used for both purposes. Another class would be introduced for
executing parallel genetic algorithm which would accept multiple instances
of AbstractGeneticAlgorithm class and converge them in parallel. Users who
does not care for robustness would go for current implementations of the
algorithm with single population. For a better optimization quality users
would chose the new class.
[...]
Thanks & Regards
--Avijit Basak
On Mon, 14 Feb 2022 at 15:37, Gilles Sadowski <gillese...@gmail.com> wrote:
> 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
>
>
