[
https://issues.apache.org/jira/browse/IGNITE-6899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16333520#comment-16333520
]
Turik Campbell edited comment on IGNITE-6899 at 1/21/18 1:42 PM:
-----------------------------------------------------------------
[~techbysample], Please review my changes.
Addressed the following aforementioned comments:
"..Bad news are, there are multiple deviations from coding style guidelines of
Ignite, 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..."
>>Turik C: Fixed. Applied coding style.
"...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)..."
>>Turik C: Fixed
"..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..."
>>Turik C: Fixed
"..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.."
>>Turik C: I am not why this is an issue. Additional discussion is necessary.
"..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.''
Turik C: Fixed, Corrected spelling, updated visibility to private.
"..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)..."
>>Turik C: Fixed. Implemented in class: OptimizeMakeChangeGAExample
"..(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.."
>>Turik C: Fixed
"..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).
>>Turik C: Fixed. Implemented in favor of
>>GAConfiguration.isHigherFitnessValueFitter() as boolean type.
"..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 an inappropriate HTML line break within phrase "terminate when (line break
here) a Chromosome's.."
>>Turik C: Fixed. updated documentation:
>>[https://apacheignite.readme.io/v2.3/docs/genetic-algorithms]
was (Author: netmille):
[~*techbysample*], Please review my changes.
Addressed the following aforementioned comments:
"..Bad news are, there are multiple deviations from coding style guidelines of
Ignite, 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..."
>>Turik C: Fixed. Applied coding style.
"...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)..."
>>Turik C: Fixed
"..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..."
>>Turik C: Fixed
"..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.."
>>Turik C: I am not why this is an issue. Additional discussion is necessary.
"..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.''
Turik C: Fixed, Corrected spelling, updated visibility to private.
"..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)..."
>>Turik C: Fixed. Implemented in class: OptimizeMakeChangeGAExample
"..(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.."
>>Turik C: Fixed
"..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).
>>Turik C: Fixed. Implemented in favor of
>>GAConfiguration.isHigherFitnessValueFitter() as boolean type.
"..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 an inappropriate HTML line break within phrase "terminate when (line break
here) a Chromosome's.."
>>Turik C: Fixed. updated documentation:
>>https://apacheignite.readme.io/v2.3/docs/genetic-algorithms
> 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
> Priority: Major
> Fix For: 2.5
>
> 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
(v7.6.3#76005)
