Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IGNITE-7877: Improve code style in GA part #3695

Closed
wants to merge 15 commits into from
Closed

IGNITE-7877: Improve code style in GA part #3695

wants to merge 15 commits into from

Conversation

techbysample
Copy link
Contributor

No description provided.

ybabak
ybabak suggested changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@ybabak ybabak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check comments and remove empty lines between class declaration and the first field for all classes.

modules/ml/src/main/java/org/apache/ignite/ml/genetic/Chromosome.java
@@ -46,7 +48,6 @@
private long[] genes;

/**
*
* @param genes Primary keys of Genes
*/
public Chromosome(long[] genes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed javadoc for line 103

/** {@inheritDoc} */
    @Override public String toString() {

modules/ml/src/main/java/org/apache/ignite/ml/genetic/GAGrid.java Outdated
orderDirection = "asc";
}
if (config.isHigherFitnessValueFitter() == false) {
orderDirection = "asc";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

braces style and this expression may be improved:

if (!config.isHigherFitnessValueFitter())
            orderDirection = "asc";

modules/ml/src/main/java/org/apache/ignite/ml/genetic/GAGrid.java Outdated
@@ -230,10 +231,9 @@ public Chromosome evolve() {
List<Long> orderChromKeysByFittest = new ArrayList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed diamond?
List<Long> orderChromKeysByFittest = new ArrayList<>();

modules/ml/src/main/java/org/apache/ignite/ml/genetic/Gene.java Outdated
@@ -36,6 +36,7 @@
*/
public class Gene {

/** primary key of Gene */
private static final AtomicLong ID_GEN = new AtomicLong();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed javadoc for toString()

modules/ml/src/main/java/org/apache/ignite/ml/genetic/MutateJob.java
@@ -42,6 +42,7 @@
/** primary keys of genes to be used in mutation **/
private List<Long> mutatedGeneKeys;

/** Ignite instance */
@IgniteInstanceResource
private Ignite ignite = null;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brace style for line 80

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And TODO at the line 69

modules/ml/src/main/java/org/apache/ignite/ml/genetic/MutateTask.java Outdated
@@ -49,9 +49,11 @@
*/
public class MutateTask extends ComputeTaskAdapter<List<Long>, Boolean> {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line here and braces at the lines 145 and 148

modules/ml/src/main/java/org/apache/ignite/ml/genetic/TruncateSelectionJob.java
@@ -42,6 +42,7 @@
/** primary keys of genes to be used in mutation */
private List<Long> mutatedGeneKeys;

/** Ignite instance */
@IgniteInstanceResource
private Ignite ignite = null;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO at the line 64 and double braces at the line 72

modules/ml/src/main/java/org/apache/ignite/ml/genetic/TruncateSelectionTask.java Outdated
@@ -44,9 +44,9 @@
/**
* Responsible for performing truncate selection.
*/

public class TruncateSelectionTask extends ComputeTaskAdapter<List<Long>, Boolean> {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line here and please format methods getChromosome and getEnhancedPopulation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And TODO in reduce method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ybabak, Please share with me what respective tools you are using to validate code style. Presently, I am using IntelliJ's format command to apply the respective style using 'ignite_codeStyle.xml'

IE:

format -s \ignite_codeStyle.xml -m *.java -r \modules\ml\src\main\java\org\apache\ignite\ml\genetic

ybabak
ybabak suggested changes Apr 4, 2018
View reviewed changes
Copy link
Contributor

@ybabak ybabak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check comments

modules/ml/src/main/java/org/apache/ignite/ml/genetic/functions/GAGridFunction.java Outdated
@@ -35,7 +35,6 @@
* Responsible for providing custom SQL functions to retrieve optimization results
*/
public class GAGridFunction {

/** GAConfiguration **/
private GAConfiguration config = null;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for lines 52 and 63: are those lines is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. These line are required.

modules/ml/src/main/java/org/apache/ignite/ml/genetic/parameter/GAGridConstants.java Outdated
@@ -34,4 +33,5 @@
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please cleanup lines 34-36

Copy link
Contributor Author

@techbysample techbysample Apr 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is they way source was formatted after applying IntelliJ's format and ignite_codeStyle.xml on source. - Fixed.

modules/ml/src/main/java/org/apache/ignite/ml/genetic/CrossOverJob.java
@IgniteInstanceResource
private Ignite ignite = null;

/** Ignite logger */
@LoggerResource
private IgniteLogger log = null;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check line 87

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

modules/ml/src/main/java/org/apache/ignite/ml/genetic/GAGrid.java
@@ -79,7 +78,6 @@ public GAGrid(GAConfiguration config, Ignite ignite) {
*
* @return Average fitness score
*/

private Double calculateAverageFitness() {

double avgFitnessScore = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 105: Do we need return value here?
And the same for line 122, 158, 310, 428

And for 148(+ 404): unnecessary local variable

Brace style for lines 362 and 365

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 105. Fixed
Line 122, 158, 310, 428. Fixed

148(+ 404): - These variable are necessary.
Brace style for lines 362 and 365. Fixed

modules/ml/src/main/java/org/apache/ignite/ml/genetic/MutateTask.java Outdated
config.getChromosomeLength();

for (int i = 0; i < config.getChromosomeLength(); i++) {
// Gene gene=config.getGenePool().get(selectRandomIndex(config.getGenePool().size()));
mutatedGenes.add(selectGene(i));
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brace style for lines 143 and 146

And for 187: unnecessary local variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brace style for lines 143 and 146. Fixed.

And for 187: unnecessary local variable - This variable is necessary.

@asfgit asfgit closed this in b97b1ee Apr 8, 2018
asfgit pushed a commit that referenced this pull request Apr 25, 2018
@techbysample @ybabak
IGNITE-7877: Improve code style in GA part
f2c5f11 
this closes #3695

(cherry picked from commit b97b1ee)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ybabak ybabak ybabak requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@techbysample @ybabak