ignite-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Turik Campbell (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (IGNITE-6899) Adding GA Grid to Apache Ignite ML module.
Date Sat, 20 Jan 2018 20:44:00 GMT

    [ https://issues.apache.org/jira/browse/IGNITE-6899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16333314#comment-16333314
] 

Turik Campbell edited comment on IGNITE-6899 at 1/20/18 8:43 PM:
-----------------------------------------------------------------

Addressed the following aforementioned comments:

 "..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..."

>>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):
Addressed the following aforementioned comments:

 

"...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)

Mime
View raw message