[
https://issues.apache.org/jira/browse/IGNITE-6899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16320924#comment-16320924
]
Oleg Ignatenko edited comment on IGNITE-6899 at 1/12/18 3:57 PM:
-----------------------------------------------------------------
I copied [this branch|https://github.com/techbysample/ignite/tree/ignite-6899]
to my machine and did a couple of brief checks. Overall, it looks good.
Recommend to put an effort into integrating this functionality in Ignite,
specifically adjust the code to closer match project coding guidelines and
after it's done perform one more round of design review.
I have a strong feeling that code and design are mostly good enough and won't
need substantial changes in order to be integrated.
If needed, refer notes below for more details.
- Checked builds of ml module and examples from command line, these went
smoothly (my builds were without tests and javadocs though).
- Run examples (from IDEA), all executed smoothly: HelloWorld, Movie (with
{{-DGENRES=Action,Comedy,Romance}}), OptimizeMakeChange (with
{{-DAMOUNTCHANGE=75}}).
- Briefly studied examples code. In examples pom, ignite-ml dependency outside
of ml profile looks superfluous but that's minor. Wrt java code of examples,
good news are that it loos clear and easy to understand, just the kind code I'd
want to see when learning new API. Very well done. Bad news are, there are
multiple deviations from [coding style guidelines of
Ignite|https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines], I
am not even sure if it can pass javadocs check at Teamcity. These issues look
manageable though and given that examples code is so apparently useful I would
strongly recommend to put an effort into onboarding it to the project.
- Run unit tests (GAGridTestSuite, in IDEA) - all passed.
- Run coverage analysis of unit tests (report attached: [^coverage.zip]). Per
cursory glance, coverage looks reasonably okay - similar or better to that in
most other ml packages. It's not as good as that of math package but as far as
I know improving unit tests coverage in whole ml module is planned as dedicated
effort anyway so it's probably good enough for now.
- Gave a first read to code added to ml module (note I plan to re-read it later
so may add more comments besides notes below):
{panel}Generally I've got same impression as of examples: code mostly looks
well designed and structured and easy to read but there are multiple deviations
from Ignite coding guidelines.
Another sad thing I noticed is repeated errors in javadocs of the kind when
method parameter type is written instead of name (for example in javadoc for
{{Chromosome#setFitnessScore}}).
Yet another javadoc related concern (though one that is easy to fix) is
{{package-info.java}} files are missing in sub-packages of genetic (sub-folders
of src/main/java/org/apache/ignite/ml/genetic) - if not fixed this will break
building javadocs of the project if I remember correctly.
Another thing I'm not entirely comfortable with is usage of enums in
GAGridConstants; it looks familiar and idiomatic for pre-Java 8 code but our
experience in ml module so far suggests that using Java 8 functional way
instead of enums often works better for stuff like that (for examples refer eg
to {{org.apache.ignite.ml.math.functions.Functions}}). Possibly worth giving it
a try.
There is repeated typo "criteria" -> "critiera" in various places - and what is
concerning is shows even in some names of public methods, eg
{{GAConfiguration#getChromosomeCritiera}}.
In GACongiguration, it would be nice to add a unit test or example with
non-default value of {{elitismCount}} (myself I would prefer unit test but I
don't insist on that).
(maybe minor but still) In class ChromosomeCriteria field {{criteria}} is
default visibility but presence of setter and getter suggests that it's
supposed to be private.{panel}
- Gave a second read to implementation code, this time more of a higher level
view, trying to ignore minor formal issues. From this perspective design looks
solid (except for that enums issue mentioned in previous comments; I drilled
into their usage a little bit deeper and it really feels somehow suboptimal -
eg {{FITNESS_EVALUATER_TYPE}} seems to have boolean semantics, not that of
enum). That said, I would recommend yet another round design review after
adjusting the code style to better fit project guidelines because currently
style deviations tend to blur perception and make it harder to evaluate design.
- Сhecked [documentation at
readme.io|https://apacheignite.readme.io/v2.3/docs/genetic-algorithms] - it
seems to be in a _very_ good shape. I only noticed two minor issues: 1) "Apache
Ignite 2.x release has expanded..." reads confusing because this is not yet
integrated. I'd rephrase to something like "Apache Ignite community is working
on expanding..." (this wording of course will need an update after GA Grid is
integrated). 2) In section "Define terminate condition" there seem to be in
inappropriate HTML line break within phrase "terminate when _(line break here)_
a Chromosome's".
was (Author: oignatenko):
I copied [this branch|https://github.com/techbysample/ignite/tree/ignite-6899]
to my machine and did a couple of brief checks. Overall, so far so good (though
I am not 100% done yet).
- I checked builds of ml module and examples from command line, these went
smoothly (my builds were without tests and javadocs though).
- Run examples (from IDEA), all executed smoothly: HelloWorld, Movie (with
{{-DGENRES=Action,Comedy,Romance}}), OptimizeMakeChange (with
{{-DAMOUNTCHANGE=75}}).
- Briefly studied examples code. In examples pom, ignite-ml dependency outside
of ml profile looks superfluous but that's minor. Wrt java code of examples,
good news are that it loos clear and easy to understand, just the kind code I'd
want to see when learning new API. Very well done. Bad news are, there are
multiple deviations from [coding style guidelines of
Ignite|https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines], I
am not even sure if it can pass javadocs check at Teamcity. These issues look
manageable though and given that examples code is so apparently useful I would
strongly recommend to put an effort into onboarding it to the project.
- Run unit tests (GAGridTestSuite, in IDEA) - all passed.
- Run coverage analysis of unit tests (report attached: [^coverage.zip]). Per
cursory glance, coverage looks reasonably okay - similar or better to that in
most other ml packages. It's not as good as that of math package but as far as
I know improving unit tests coverage in whole ml module is planned as dedicated
effort anyway so it's probably good enough for now.
- I gave a first read to code added to ml module (note I plan to re-read it
later so may add more comments besides notes below):
{panel}Generally I've got same impression as of examples: code mostly looks
well designed and structured and easy to read but there are multiple deviations
from Ignite coding guidelines.
Another sad thing I noticed is repeated errors in javadocs of the kind when
method parameter type is written instead of name (for example in javadoc for
{{Chromosome#setFitnessScore}}).
Yet another javadoc related concern (though one that is easy to fix) is
{{package-info.java}} files are missing in sub-packages of genetic (sub-folders
of src/main/java/org/apache/ignite/ml/genetic) - if not fixed this will break
building javadocs of the project if I remember correctly.
Another thing I'm not entirely comfortable with is usage of enums in
GAGridConstants; it looks familiar and idiomatic for pre-Java 8 code but our
experience in ml module so far suggests that using Java 8 functional way
instead of enums often works better for stuff like that (for examples refer eg
to {{org.apache.ignite.ml.math.functions.Functions}}). Possibly worth giving it
a try.
There is repeated typo "criteria" -> "critiera" in various places - and what is
concerning is shows even in some names of public methods, eg
{{GAConfiguration#getChromosomeCritiera}}.
In GACongiguration, it would be nice to add a unit test or example with
non-default value of {{elitismCount}} (myself I would prefer unit test but I
don't insist on that).
(maybe minor but still) In class ChromosomeCriteria field {{criteria}} is
default visibility but presence of setter and getter suggests that it's
supposed to be private.{panel}
-----
Further I plan to check [documentation at
readme.io|https://apacheignite.readme.io/v2.3/docs/genetic-algorithms]. After
that I plan to give a second read to code added in {{genetic}} packages in ml
module, under both main and test.
I am going to continue posting additional observations and considerations here.
> Adding GA Grid to Apache Ignite ML module.
> ------------------------------------------
>
> Key: IGNITE-6899
> URL: https://issues.apache.org/jira/browse/IGNITE-6899
> Project: Ignite
> Issue Type: New Feature
> Components: ml
> Reporter: Yury Babak
> Assignee: Turik Campbell
> Fix For: 2.4
>
> Attachments: coverage.zip
>
>
> We want to add GA Grid to our ML Module.
> This is the first iteration of this integration. On this step we will simple
> add GA Grid to the separate package in ML module.
> (i) This is a good package for GA Grid: org.apache.ignite.ml.genetic
> (i) For GA Grid we need unit tests as well as examples
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
