You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ignite.apache.org by "Oleg Ignatenko (JIRA)" <ji...@apache.org> on 2018/01/11 19:09:00 UTC
[jira] [Comment Edited] (IGNITE-6899) Adding GA Grid to Apache
Ignite ML module.
[ 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/11/18 7:08 PM:
-----------------------------------------------------------------
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.
was (Author: oignatenko):
I copied this branch 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.
-----
Above are first brief observations. Further I plan to closer study code added in {{genetic}} packages in ml module, under both main and test and check [documentation at readme.io|https://apacheignite.readme.io/v2.3/docs/genetic-algorithms]. 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)