Calculate Geometric and Arithmetic Mean

Question

I am trying to make general class to calculate Geometric mean using exponential of the arithmetic mean of logarithms , and Arithmetic Mean.

I am looking for some general feedback on how I can improve the structure and efficiency of my code.

package analysis.statistic;

import java.util.Arrays;

public class Mean {

/**
 * this function calculate geometric mean using the exponential of the
 * arithmetic mean of logarithms
 * 
 * <ul>
 * <li>(-1)^m * 1/n-rt(product(numbers)) = (-1)^m exp(1/n
 * sum(ln(numbers[i])))
 * <li>n : is length of numbers</li>
 * <li>m : is number of negative values</li>
 * </ul>
 * 
 * @param numbers
 * @return geometric mean
 *         <ul>
 *         <li>NAN : if numbers array is empty</li>
 *         <li>0 :if numbers array contain 0 value</li>
 *         <li>negative value : if numbers array contains odd negative
 *         values
 *         <li>positive value : if numbers array contains even negative
 *         values or just positive values
 *         </ul>
 * 
 * @throws IllegalArgumentException
 *             if numbers array are null
 */
public static double geometricMean(int... numbers) {
    if (numbers == null) {
        throw new IllegalArgumentException("numbers must be not null");
    }

    if (numbers.length == 0) {
        return Double.NaN;
    }

    int nNegativeValues = 0;
    double logarithmSum = 0;

    for (int i : numbers) {
        if (i > 0) {
            logarithmSum += Math.log(i);

        } else if (i < 0) {
            nNegativeValues += 1;
            logarithmSum += Math.log(i * -1);
        } else if (i == 0) {
            return 0;
        }
    }

    int length = numbers.length;

    return expOfArithMeanOfLogs(nNegativeValues, logarithmSum, length);

}

/**
 * Works just like {@link Mean#geometricMean(int...)} except the array
 * contains long numbers
 */
public static double geometricMean(long... numbers) {

    if (numbers == null) {
        throw new IllegalArgumentException("numbers must be not null");
    }

    if (numbers.length == 0) {
        return Double.NaN;
    }

    int nNegativeValues = 0;
    double logarithmSum = 0;

    for (long i : numbers) {
        if (i > 0) {
            logarithmSum += Math.log(i);

        } else if (i < 0) {
            nNegativeValues += 1;
            logarithmSum += Math.log(i * -1);
        } else if (i == 0) {
            return 0;
        }
    }

    return expOfArithMeanOfLogs(nNegativeValues, logarithmSum, numbers.length);

}

/**
 * Works just like {@link Mean#Mean#geometricMean(int...)} except the array
 * contains double numbers
 */
public static double geometricMean(double... numbers) {

    if (numbers == null) {
        throw new IllegalArgumentException("numbers must be not null");
    }

    if (numbers.length == 0) {
        return Double.NaN;
    }

    int nNegativeValues = 0;
    double logarithmSum = 0;

    for (double i : numbers) {
        if (i > 0) {
            logarithmSum += Math.log(i);

        } else if (i < 0) {
            nNegativeValues += 1;
            logarithmSum += Math.log(i * -1);
        } else if (i == 0) {
            return 0;
        }
    }

    return expOfArithMeanOfLogs(nNegativeValues, logarithmSum, numbers.length);

}

/**
 * Works just like {@link Mean#geometricMean(int...)} except the array
 * contains float numbers
 */
public static double geometricMean(float... numbers) {

    if (numbers.length == 0) {
        return Double.NaN;
    }

    int nNegativeValues = 0;
    double logarithmSum = 0;

    for (float i : numbers) {
        if (i > 0) {
            logarithmSum += Math.log(i);

        } else if (i < 0) {
            nNegativeValues += 1;
            logarithmSum += Math.log(i * -1);
        } else if (i == 0) {
            return 0;
        }
    }

    return expOfArithMeanOfLogs(nNegativeValues, logarithmSum, numbers.length);

}

/**
 * Return Exponential of the arithmetic mean of logarithms
 * 
 * @param m
 *            is the number of negative numbers
 * @param logarithmSum
 *            is arithmetic mean of logarithms
 * @param n
 *            numbers of values
 * @return exponential of the arithmetic mean of logarithms
 */
private static double expOfArithMeanOfLogs(int m, double logarithmSum, int n) {

    double expOfLogarithms = Math.exp(((double) 1 / n) * logarithmSum);

    if (m != 0) {
        expOfLogarithms = expOfLogarithms * Math.pow(-1, m);
    }
    return expOfLogarithms;
}

/**
 * The mean is the average of the numbers.
 * <li>sum(numbers[i])/n</li>
 * <li>n : length of array numbers</li>
 * 
 * @param numbers
 *            integers
 * @return average of the numbers
 *         <ul>
 *         <li>NAN : if array numbers is empty
 *         <li>average : if contains numbers
 *         </ul>
 * @throws IllegalArgumentException
 *             if numbers array is null
 */
public static double arithmeticMean(int... numbers) {
    if (numbers == null) {
        throw new IllegalArgumentException("numbers must be not null");
    }
    if (numbers.length == 0) {
        return Double.NaN;
    }
    return (double) Arrays.stream(numbers).parallel().sum() / numbers.length;

}

/**
 * Works just like {@link Mean#arithmeticMean(int...)} except the array
 * contains double numbers and
 * 
 * @param numbers
 *             double
 */
public static double arithmeticMean(double... numbers) {
    if (numbers == null) {
        throw new IllegalArgumentException("numbers must be not null");
    }
    if (numbers.length == 0) {
        return Double.NaN;
    }
    return (double) Arrays.stream(numbers).parallel().sum() / numbers.length;
}

/**
 * Works just like {@link Mean#arithmeticMean(int...)} except the array
 * contains long numbers and
 * 
 * @param numbers
 *             long
 */
public static double arithmeticMean(long... numbers) {
    if (numbers == null) {
        throw new IllegalArgumentException("numbers must be not null");
    }
    if (numbers.length == 0) {
        return Double.NaN;
    }
    return (double) Arrays.stream(numbers).parallel().sum() / numbers.length;
}

}

Answer 1

In general, your code looks fine. Here are some suggestions:

• In chained if-else clauses where the final if-clause is supposed to represent all cases that are not covered by the preceding if-clauses, like this one:

  if (i > 0) {
      // ...
  } else if (i < 0) {
      // ...
  } else if (i == 0) {
      // ...
  }
  

  It is less ambiguous to omit the final if clause and make the last condition a simple else without an if. That way, not only you, but also the compiler knows that this is supposed to be the "default" case, which means the structure of the code represents your intention more closely. To confirm your assumption that the default case is indeed only whenever i == 0, you can use an assertion instead:

  if (i > 0) {
      // ...
  } else if (i < 0) {
      // ...
  } else {
      assert i == 0;
      // ...
  }
  

  If you write your code like this, then an AssertionError will, should the situation arise, alert you to the fact that, when i is either a float or a double, !(i > 0) && !(i < 0) is actually not equivalent to i == 0, because i might represent Float.NaN or Double.NaN, in which case neither of the three conditions in your original code will evaluate to true. This means that, if somebody slips a NaN into the arguments, your method will only sum the logarithms of all non-NaN input values, but will calculate the mean based on the total number of arguments, including the NaN-values, thus producing an erroneous result (not to mention the scenario where the input array consists entirely of NaNs).

• You can reduce code duplication by coding the algorithm calculating the geometric mean for double... and long... input values only and delegating the methods for int... and float... to those two methods. It seems that, since the advent of Java 8, conversions like int[] to long[] are actually pretty easy.

  Actually, you could even delegate the long... method to the double... method. While a conversion from long to double might be lossy, the only thing you are doing with the input values is passing them to Math.log(double), so they will be converted to a double anyway before any calculations are performed.

• Here:

  if (m != 0) {
      expOfLogarithms = expOfLogarithms * Math.pow(-1, m);
  }
  

  The if clause is redundant, because any number except 0 raised to the power of 0 is 1, so the effect of the code with be the same without the if clause.

• About the documentation of geometricMean(int...):
  • The general description says that it calculates the geometric mean "using the exponential of the arithmetic mean of logarithms". This is true only if the numbers don't contain zero. You might update the general description to specify that, regardless of what the description for return says.
  • The wording "if numbers array contains odd negative values" is confusing, because it's not about whether the values themselves are odd, but whether the number of negative values is odd (same goes for the description when a positive value is returned).
  • Zero is an even number, so you don't need to list this case explicitly in the description for when a positive value is returned.
• You forgot to check for null in geometricMean(float...).
• You can omit the class name in a {@link} tag if you link to the same class, so {@link #geometricMean(int...)} will be valid as well (by the way, the link in the documentation of geometricMean(double...) is faulty).

• There is a shortcut for checking arguments against being null:

  Objects.requireNonNull(numbers, "numbers must be not null");
  

  This will throw a NullPointerException instead of an IllegalArgumentException.

• In arithmeticMean(double...), you don't need to cast the sum of the stream to double, because DoubleStream.sum() already returns a double.

Answer 2

You are currently repeating the code unnecessarily. Instead of writing the same algorithm three times, you should define an inner helper class like this:

private static class Stats {
    private int n;
    private double logsum;
    private double sign = 1.0;

    void add(double num) {
        n++;
        if (num < 0) {
            logsum += Math.log(-num);
            sign = -sign;
        } else {
            logsum += Math.log(num);
        }
    }

    double getGeometricMean() {
        return sign * Math.exp(logsum / n);
    }
}

Instead of counting the number of negative numbers, you only need to remember whether it was an even or odd number of numbers. Therefore the sign field toggles between 1.0 and -1.0.

Then you can use this class in all three methods, avoiding duplicate code:

public static double geometricMean(int... numbers) {
    Stats stats = new Stats();
    for (int number : numbers) {
        if (number == 0) {
            return 0.0;
        }
        stats.add(number);
    }
    return stats.getGeometricMean();
}

---

The Javadoc of all the methods should be consistent. In geometricMean(int...) it starts with this function calculate geometric, which is wrong English.

• Conventionally, Javadoc starts with an uppercase letter.
• The this function is redundant and should be left out.
• The calculate has to be calculates, like in the other methods.
• The Javadoc for methods and classes ends with a full stop.
• There should be no space before the colon.
• Having an empty @param numbers without any further explanation is useless. It just states that there is a parameter of that name, which is clearly visible from the code. So either describe the parameter, or remove that empty @param declaration.
• The Javadoc should describe the effects of calling the method, not the implementation details. Your use of logarithms can be considered an implementation detail.

So the Javadoc should look like:

/**
 * Calculates the geometric mean of the given numbers.
 *
 * @return ...
 */

The Javadoc for arithmeticMean(int...) is missing the <ul> tags around the <li> tags.

The code for arithmeticMean currently looks like this:

public static double arithmeticMean(long... numbers) {
    if (numbers == null) {
        throw new IllegalArgumentException("numbers must be not null");
    }
    if (numbers.length == 0) {
        return Double.NaN;
    }
    return (double) Arrays.stream(numbers).parallel().sum() / numbers.length;
}

I would write the following instead:

public static double arithmeticMean(long... numbers) {
    return (double) Arrays.stream(numbers).sum() / numbers.length;
}

I left out the null check since even without the check a NullPointerException is thrown pretty early by the Arrays.stream method.

I left out the special case for length == 0, since 0.0 / 0 is defined to be NaN.

I removed the .parallel() call since summing up doubles can lead to different results depending on the exact order.

If you want reproducible results, use StrictMath.log/exp instead of Math.log/exp.

---

Depending on your needs for precision, you should look at the Kahan summation algorithm, so that you don't lose any precision while adding up all the logarithms.

---

The Javadocs for arithmeticMean have unfinished sentences.