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.

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

Feature/math 1563 #197

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open

Conversation

avijitbasak
Copy link

Changes for task MATH-1618

@avijitbasak avijitbasak marked this pull request as ready for review September 6, 2021 13:48
@aherbert
Copy link
Contributor

aherbert commented Sep 6, 2021

Thanks for the contribution. There are a few issues to address before a detailed review.

This will fail on checkstyle. You should replace all tab characters with 4 spaces and add new lines at the end of files. You can run a checkstyle report using:

mvn checkstyle:checkstyle

And then open target/site/checkstyle.html.

Before the CI build will pass you can check that the local build passes the default goal. This is:

mvn clean verify apache-rat:check checkstyle:check pmd:check spotbugs:check javadoc:javadoc

You can run the code check targets separately to get a report on what is wrong (outputs will be in the target/site directory):

mvn checkstyle:checkstyle
mvn pmd:pmd
mvn spotbugs:spotbugs

In general you should check that the module passes the default maven goal:

mvn

You have public interfaces with redundant public keywords on methods or properties.

There is a string property named CHROMOZOME which should be corrected.

The module directory name commons-math4-genetics should be commons-math-genetics. The 4 is used only in the artifactId.

The class RandomGenerator has a synchronized method to access a singleton. The effect of the synchronized keyword is meaningless here. The returned singleton is not thread-safe. A different design is required if the object is intended to be used across threads. A better approach is a single random generator per thread. The WELL_19937_C generator is not a very robust generator. There are faster and statistically better random generators available. For cross thread generic use you could try for example ThreadLocalRandomSource.current(RandomSource.XO_RO_SHI_RO_128_PP). If you want a thread-pool to avoid sequence collisions/overlap on the random output from the generator used by each thread then this will require creating threads with a child generator, each created from the same parent JumpableUniformRandomProvider.

@avijitbasak
Copy link
Author

avijitbasak commented Sep 20, 2021

The following errors have been received after build. These are due to formatting of lambda expression. I have formatted the manually. It will be helpful if anyone can share eclipse configuration to resolve this.

[INFO] There are 6 errors reported by Checkstyle 8.29 with /home/travis/build/apache/commons-math/commons-math4-genetics/../src/main/resources/checkstyle/checkstyle.xml ruleset.
[ERROR] src/test/java/org/apache/commons/math4/genetics/selection/TournamentSelectionTest.java:[51] (indentation) Indentation: 'block' child has incorrect indentation level 16, expected level should be 20.
[ERROR] src/test/java/org/apache/commons/math4/genetics/selection/TournamentSelectionTest.java:[52] (indentation) Indentation: 'block rcurly' has incorrect indentation level 12, expected level should be 16.
[ERROR] src/test/java/org/apache/commons/math4/genetics/dummy/DummyChromosome.java:[27] (indentation) Indentation: 'block' child has incorrect indentation level 12, expected level should be 16.
[ERROR] src/test/java/org/apache/commons/math4/genetics/dummy/DummyChromosome.java:[28] (indentation) Indentation: 'block rcurly' has incorrect indentation level 8, expected level should be 12.
[ERROR] src/test/java/org/apache/commons/math4/genetics/utils/ChromosomeRepresentationUtilsTest.java:[114] (indentation) Indentation: 'block' child has incorrect indentation level 20, expected level should be 12.
[ERROR] src/test/java/org/apache/commons/math4/genetics/utils/ChromosomeRepresentationUtilsTest.java:[115] (indentation) Indentation: 'block rcurly' has incorrect indentation level 16, expected level should be 8.
[INFO] ------------------------------------------------------------------------

@avijitbasak avijitbasak reopened this Sep 20, 2021
@coveralls
Copy link

coveralls commented Sep 24, 2021

Coverage Status

Coverage decreased (-0.04%) to 90.418% when pulling 4b094b2 on avijitbasak:feature/MATH-1563 into 470bdbb on apache:master.

<artifactId>commons-math-parent</artifactId>
<version>4.0-SNAPSHOT</version>
</parent>
<artifactId>commons-math4-ga</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the 4 from math4. The version is specified separately from the artifact ID.

Copy link
Author

Choose a reason for hiding this comment

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

Done and committed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants