Skip to content

Refactoring changes #129

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactoring changes #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions commons-numbers-angle/src/main/java/org/apache/commons/numbers/angle/Angle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
* Represents the <a href="https://en.wikipedia.org/wiki/Angle">angle</a> concept.
*/
public abstract class Angle implements DoubleSupplier {
/** 2&pi;. */
public static final double TWO_PI = 2 * Math.PI;
/** &pi;/2. */
public static final double PI_OVER_TWO = 0.5 * Math.PI;
/** Turns to degrees conversion factor. */
Expand Down Expand Up @@ -92,6 +90,8 @@ public boolean equals(final Object other) {
* Unit: <a href="https://en.wikipedia.org/wiki/Turn_%28geometry%29">turns</a>.
*/
public static final class Turn extends Angle {
/** 2&pi;. */
public static final double TWO_PI = 2 * Math.PI;
/** Zero. */
public static final Turn ZERO = Turn.of(0d);
/** Normalizing operator (result will be within the {@code [0, 1[} interval). */
Expand Down Expand Up @@ -146,12 +146,15 @@ public static DoubleUnaryOperator normalizer(final double lo) {
* Unit: <a href="https://en.wikipedia.org/wiki/Radian">radians</a>.
*/
public static final class Rad extends Angle {
/** 2&pi;. */
public static final double ANGLE_TWO_PI = 2 * Math.PI;

/** Zero. */
public static final Rad ZERO = Rad.of(0d);
/** &pi;. */
public static final Rad PI = Rad.of(Math.PI);
/** 2&pi;. */
public static final Rad TWO_PI = Rad.of(Angle.TWO_PI);
public static final Rad TWO_PI = Rad.of(ANGLE_TWO_PI);
/** Normalizing operator (result will be within the <code>[0, 2&pi;[</code> interval). */
public static final DoubleUnaryOperator WITHIN_0_AND_2PI = normalizer(0d);
/** Normalizing operator (result will be within the <code>[-&pi;, &pi;[</code> interval). */
Expand All @@ -175,7 +178,7 @@ public static Rad of(final double angle) {
/** {@inheritDoc} */
@Override
public Turn toTurn() {
return Turn.of(getAsDouble() / Angle.TWO_PI);
return Turn.of(getAsDouble() / ANGLE_TWO_PI);
}

/** {@inheritDoc} */
Expand All @@ -198,7 +201,7 @@ public Deg toDeg() {
* @return the normalization operator.
*/
public static DoubleUnaryOperator normalizer(final double lo) {
return new Normalizer(lo, Angle.TWO_PI);
return new Normalizer(lo, ANGLE_TWO_PI);
}
}

Expand Down
6 changes: 3 additions & 3 deletions commons-numbers-angle/src/test/java/org/apache/commons/numbers/angle/AngleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ void testConstants() {
Assertions.assertEquals(0d, Angle.Deg.ZERO.getAsDouble());
Assertions.assertEquals(Math.PI, Angle.Rad.PI.getAsDouble());
Assertions.assertEquals(2 * Math.PI, Angle.Rad.TWO_PI.getAsDouble());
Assertions.assertEquals(2 * Math.PI, Angle.TWO_PI);
Assertions.assertEquals(2 * Math.PI, Angle.Turn.TWO_PI);
Assertions.assertEquals(Math.PI / 2, Angle.PI_OVER_TWO);
}

Expand Down Expand Up @@ -133,11 +133,11 @@ void testNormalizeVeryCloseToBounds() {

// arrange
final double pi = Math.PI;
double small = Math.ulp(Angle.TWO_PI);
double small = Math.ulp(Angle.Turn.TWO_PI);
double tiny = 5e-17; // pi + tiny = pi (the value is too small to add to pi)

// act/assert
Assertions.assertEquals(Angle.TWO_PI - small, nPi.applyAsDouble(-small));
Assertions.assertEquals(Angle.Turn.TWO_PI - small, nPi.applyAsDouble(-small));
Assertions.assertEquals(small, nPi.applyAsDouble(small));

Assertions.assertEquals(pi - small, nZero.applyAsDouble(-pi - small));
Expand Down
46 changes: 18 additions & 28 deletions ...mples-jmh/src/main/java/org/apache/commons/numbers/examples/jmh/gamma/ErfPerformance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,16 @@ protected double[] createNumbers(SplittableRandom rng) {
* Contains an array of numbers and the method to compute the error function.
*/
public abstract static class FunctionData extends NumberData {
/** The type of the data.
Should be num uniform
Declared in ErfPerformance class. **/
@Param({NUM_UNIFORM})
private String NUM_TYPE;
Copy link
Contributor

@aherbert aherbert Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The previous benchmark had a single variable type in each @State class. By splitting this into two you will cause JMH to run the benchmark with more variables. JMH creates benchmarks as the cross-product of all variables. Thus one of the states will now have 1 * 2 benchmarks where previously it would be 1. This is a waste of resources. It will also create two columns in the results where the two columns have redundant information as child classes use only one of the variables.

/** The type of the data.
Should be num uniform
declared in ErfPerformance class. **/
@Param({NUM_UNIFORM, NUM_INVERSE_UNIFORM})
private String NUM_AND_INVERSE_TYPE;

/** The function. */
private DoubleUnaryOperator function;
Expand Down Expand Up @@ -208,15 +218,11 @@ public void setup() {
*/
@State(Scope.Benchmark)
public static class ErfData extends FunctionData {
/** The type of the data. */
@Param({NUM_UNIFORM, NUM_INVERSE_UNIFORM})
private String type;

/** {@inheritDoc} */
@Override
protected double[] createNumbers(SplittableRandom rng) {
DoubleSupplier generator;
if (NUM_INVERSE_UNIFORM.equals(type)) {
if (NUM_INVERSE_UNIFORM.equals(FunctionData.NUM_AND_INVERSE_TYPE)) {
// p range: [-1, 1)
// The final value is generated using the inverse erf function.
generator = () -> InverseErf.value(makeSignedDouble(rng));
Expand All @@ -225,7 +231,7 @@ protected double[] createNumbers(SplittableRandom rng) {
// Note: Values are not distinguishable from +/-1 when |x| > 6
generator = () -> makeSignedDouble(rng) * 6;
} else {
throw new IllegalStateException(UNKNOWN + type);
throw new IllegalStateException(UNKNOWN + FunctionData.NUM_AND_INVERSE_TYPE);
}
return DoubleStream.generate(generator).limit(getSize()).toArray();
}
Expand Down Expand Up @@ -270,15 +276,11 @@ protected void verify() {
*/
@State(Scope.Benchmark)
public static class ErfcData extends FunctionData {
/** The type of the data. */
@Param({NUM_UNIFORM, NUM_INVERSE_UNIFORM})
private String type;

/** {@inheritDoc} */
@Override
protected double[] createNumbers(SplittableRandom rng) {
DoubleSupplier generator;
if (NUM_INVERSE_UNIFORM.equals(type)) {
if (NUM_INVERSE_UNIFORM.equals(FunctionData.NUM_AND_INVERSE_TYPE)) {
// q range: [0, 2)
// The final value is generated using the inverse erfc function.
generator = () -> InverseErfc.value(rng.nextDouble() * 2);
Expand All @@ -288,7 +290,7 @@ protected double[] createNumbers(SplittableRandom rng) {
// Shift the range [-17, 17) to [-6, 28)
generator = () -> makeSignedDouble(rng) * 17 + 11;
} else {
throw new IllegalStateException(UNKNOWN + type);
throw new IllegalStateException(UNKNOWN + FunctionData.NUM_AND_INVERSE_TYPE);
}
return DoubleStream.generate(generator).limit(getSize()).toArray();
}
Expand Down Expand Up @@ -334,21 +336,15 @@ protected void verify() {
*/
@State(Scope.Benchmark)
public static class InverseErfData extends FunctionData {
/**
* The type of the data.
*/
@Param({NUM_UNIFORM})
private String type;

/** {@inheritDoc} */
@Override
protected double[] createNumbers(SplittableRandom rng) {
DoubleSupplier generator;
if (NUM_UNIFORM.equals(type)) {
if (NUM_UNIFORM.equals(FunctionData.NUM_TYPE)) {
// range [-1, 1)
generator = () -> makeSignedDouble(rng);
} else {
throw new IllegalStateException(UNKNOWN + type);
throw new IllegalStateException(UNKNOWN + FunctionData.NUM_TYPE);
}
return DoubleStream.generate(generator).limit(getSize()).toArray();
}
Expand Down Expand Up @@ -393,21 +389,15 @@ protected void verify() {
*/
@State(Scope.Benchmark)
public static class InverseErfcData extends FunctionData {
/**
* The type of the data.
*/
@Param({NUM_UNIFORM})
private String type;

/** {@inheritDoc} */
@Override
protected double[] createNumbers(SplittableRandom rng) {
DoubleSupplier generator;
if (NUM_UNIFORM.equals(type)) {
if (NUM_UNIFORM.equals(FunctionData.NUM_TYPE)) {
// range [0, 2)
generator = () -> rng.nextDouble() * 2;
} else {
throw new IllegalStateException(UNKNOWN + type);
throw new IllegalStateException(UNKNOWN + FunctionData.NUM_TYPE);
}
return DoubleStream.generate(generator).limit(getSize()).toArray();
}
Expand Down
28 changes: 19 additions & 9 deletions commons-numbers-primes/src/main/java/org/apache/commons/numbers/primes/Primes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,35 @@ public static boolean isPrime(int n) {
}
return SmallPrimes.millerRabinPrimeTest(n);
}

/**
* Return the number which is not a multiple of 3.
*
* @param n Positive number.
* @return number which is not a multiple of 3.
* @throws IllegalArgumentException if n &lt; 0.
*/
public static int nonMultipleOf3(int n) {
final int remainder = n % 3;
if (0 == remainder) { // if n % 3 == 0
n += 2; // n % 3 == 2
} else if (1 == remainder) { // if n % 3 == 1
n += 4; // n % 3 == 2
}
return n;
}
/**
* Return the smallest prime greater than or equal to n.
*
* @param n Positive number.
* @return the smallest prime greater than or equal to {@code n}.
* @throws IllegalArgumentException if n &lt; 0.
*/
public static int nextPrime(int n) {
if (n < 0) {
throw new IllegalArgumentException(MessageFormat.format(NUMBER_TOO_SMALL, n, 0));
}
if (n <= 2) {
return 2;
int firstPrime = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a variable does not add anything to the code. This could all be done using a comment. Note that numbers allows the magic numbers beyond the usual set of {-1, 0, 1} as the entire library is supporting numerical operations. To create constants for well used constants such as 2 is unnecessary. Note also that loading the numbers -1 to 5 are single byte code instructions. Since direct usage is more efficient, if any of these int constants is required then constants should only be used when comments cannot sufficiently clarify their usage.

return firstPrime; // 2 is first possible smallest prime greater than or equal to n, n here can be 0, 1 or 2
}
n |= 1; // make sure n is odd

Expand All @@ -81,12 +96,7 @@ public static int nextPrime(int n) {

// prepare entry in the +2, +4 loop:
// n should not be a multiple of 3
final int rem = n % 3;
if (0 == rem) { // if n % 3 == 0
n += 2; // n % 3 == 2
} else if (1 == rem) { // if n % 3 == 1
n += 4; // n % 3 == 2
}
n = nonMultipleOf3(n);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is refactoring for the sake of refactoring. The code comments explain what is occurring. The comments here also match the comments for the +2, +4 adjustment that happens a few lines below within the loop.

while (true) { // this loop skips all multiple of 3
if (isPrime(n)) {
return n;
Expand Down