Hi All
Please find my comments.
[...]
>I don't think that it's the right way to go; instantiating an
"ExecutorService"
>belongs to the GA application, not the GA library (whose relevant classes
>need "only" be thread-safe).
>There is some misunderstanding to be clarified in a dedicated discussion
>(please file a new JIRA ticket).
-- I have created a subtask under the same Jira(MATH-1563). Please share
your thoughts.
https://issues.apache.org/jira/browse/MATH-1643
>Side note: Conflicts and duplicate commits have accumulated in the
>dedicated "feature__MATH-1563__genetic_algorithm" branch.
>I did not know how to proceed in order to avoid ending up with a messy
>history in "master"; so I created a new branch (with the same name) with
>all the new GA-related files added as a single commit.
>Currently, this branch (based on your PR #205) fails the default goal,
>because of a CheckStyle issue. You shoudl always check locally that
>running "mvn" without arguments does not generate any errors.
>I also noticed that classes in "examples-ga" use "forbidden" library
>classes: "GeneticIllegalArgumentException" is an "internal" class; we
>must not advertize such classes in the example applications.
-- I have replaced the "GeneticIllegalArgumentException" by
"IllegalArgumentException".
>In general, it seems that "examples-ga" contains several classes and
>methods that do not need to be "public". This is especially true for
>classes like "MathFunction" and "Coordinate". [Having those "private"
>helps users to tell what is part of the library's functionality from what
is
>just "dummy" placeholder code.]
-- I have replaced the MathFunction and CoordinateDecoder with lambda.
However, the Coordinate class is a domain object (phenotype). So this needs
to remain public. This can be used in more than one place for the entire
application.
>Finally (for now), I've just noticed that there exist several classes named
>"MathFunction", with same implementation!
>Code duplication must be avoided, especially where we purport to display
>best practices.
-- As mentioned above this has been removed.
>The various "Standalone" classes also look quite similar; consolidating the
>"examples-ga" module (including full Javadoc) is necessary.
-- Could you please elaborate it more. IMHO as StandAlone classes are
dedicated to the specific module only, it would remain separate. Since we
have used a single domain to show utility of the different
types(adaptive/simple) of GA some classes have become similar.
>I still don't
>understand why there are "...-legacy" modules in module "examples-ga".
>If you want to offer the option of running the "old" implementation, you
>could add a "legacy" flag (as "@Option" in the "Standalone" application).
-- There was a discussion on this some time back. The sole purpose of
keeping the legacy example module is for comparison with the new
implementation. It will be easier for anyone to visualize the quality
improvement we achieved here. I don't want to mix(by legacy flag) this
anyway with the new implementation.
>Please use the new branch for all these ("cleanup") changes, as the basis
>a PR (with a *single* commit).
-- I have taken the changes and will create a new PR soon with all my
changes.
Thanks & Regards
--Avijit Basak
On Sun, 13 Mar 2022 at 06:39, Gilles Sadowski <gillese...@gmail.com> wrote:
> Hello.
>
> Le lun. 28 févr. 2022 à 07:11, Avijit Basak <avijit.ba...@gmail.com> a
> écrit :
> >
> > Hi All
> >
> > Please see my comments below.
> >
> > > [...]
> > >I just had a very quick look.
> > >IIUC, you always provide "convenience" methods (e.g. the various
> > >signatures for the "evolve" functionality).
> > >Prior to merging into "master", we should simplify and limit the
> > >discussion to the core functionality, i.e. not try and make decisions
> > >for the user (like default values, ...). Please keep the API as simple
> > >as possible
> > -- I have removed the mentioned evolve method.
> > However, I had to catch two checked exceptions (InterruptedException,
> > ExecutionException) and rethrow them. As of now I have handled them using
> > the GeneticIllegalArgumentException. I think we need to introduce another
> > exception class to handle this. Please share your thought regarding this.
>
> I don't think that it's the right way to go; instantiating an
> "ExecutorService"
> belongs to the GA application, not the GA library (whose relevant classes
> need "only" be thread-safe).
> There is some misunderstanding to be clarified in a dedicated discussion
> (please file a new JIRA ticket).
>
> Side note: Conflicts and duplicate commits have accumulated in the
> dedicated "feature__MATH-1563__genetic_algorithm" branch.
> I did not know how to proceed in order to avoid ending up with a messy
> history in "master"; so I created a new branch (with the same name) with
> all the new GA-related files added as a single commit.
>
> Currently, this branch (based on your PR #205) fails the default goal,
> because of a CheckStyle issue. You shoudl always check locally that
> running "mvn" without arguments does not generate any errors.
>
> I also noticed that classes in "examples-ga" use "forbidden" library
> classes: "GeneticIllegalArgumentException" is an "internal" class; we
> must not advertize such classes in the example applications.
> In general, it seems that "examples-ga" contains several classes and
> methods that do not need to be "public". This is especially true for
> classes like "MathFunction" and "Coordinate". [Having those "private"
> helps users to tell what is part of the library's functionality from what
> is
> just "dummy" placeholder code.]
>
> Finally (for now), I've just noticed that there exist several classes named
> "MathFunction", with same implementation!
> Code duplication must be avoided, especially where we purport to display
> best practices.
> The various "Standalone" classes also look quite similar; consolidating the
> "examples-ga" module (including full Javadoc) is necessary. I still don't
> understand why there are "...-legacy" modules in module "examples-ga".
> If you want to offer the option of running the "old" implementation, you
> could add a "legacy" flag (as "@Option" in the "Standalone" application).
>
> Please use the new branch for all these ("cleanup") changes, as the basis
> a PR (with a *single* commit). Thanks.
>
> Regards,
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
--
Avijit Basak
