-
Notifications
You must be signed in to change notification settings - Fork 20k
Added different types of Mean #4339
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General remarks:
-
treating the cases of empty input: I suggested to return
Double.NAN
. Other possibilities to consider: throw anIllegalArgumentException
or useOptional
as a return type. -
naming: what do you think to rename this class to
Means
and/or shorten the method names toarithmetic
,geometric
,harmonic
? -
input type: does it make sense to expect a more general input than
double[]
? Not only in terms of the type, but also to allow more containers (any Iterable?). -
implementation: what do you think about using
Stream.reduce()
like approach, instead of having explicit loops? For example,arithmeticMean
could be rewritten as:public static double arithmeticMean(final double[] numbers) { if (numbers.length == 0) { return Double.NAN; } final var sum = Arrays.stream(numbers).sum(); return sum / numbers.length; }
and
geometricMean
aspublic static double geometricMean(double[] numbers) { if (numbers.length == 0) { return Double.NAN; } final var product = Arrays.stream(numbers).reduce(1.0, (a, b) -> a * b); return Math.pow(product, 1.0 / numbers.length); }
Make sure to import
java.util.stream.Stream
andjava.util.Arrays
.
@vil02 I consider all your suggestion and will incorporate them. |
@vil02 please review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was nothing bad about double[] numbers
as input. My concern was that these functions could not be used, when numbers
are ArrayList<Double>
, LinkedList<Double>
, Vector<Double>
, TreeSet<Double>
or any other from many more.
For example one could consider rewriting the method to something like:
public static Double geometric(final Iterable<Double> numbers) {
var geometricConsumer = new Object() {
int count = 0;
double product = 1.0;
};
numbers.forEach(number -> {
geometricConsumer.product *= number;
++geometricConsumer.count;
//this could become something like geometricConsumer.update(number);
});
if (geometricConsumer.count == 0) {
throw new IllegalArgumentException("Empty iterable given for Mean computation.");
}
return Math.pow(geometricConsumer.product, 1.0 / geometricConsumer.count);
// and the logic above could be expressed as geometricConsumer.getValue();
}
Well... this will not work directly with double[]
.
@vil02 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the time complexity of IterableUtils.size(numbers)
? I think the iterator has to be traversed (i.e. in order to compute any mean, we traverse the iterator twice). @siriak can we live with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code and the tests look good for me (lets wait for @siriak to clarify things mentioned above).
The suggestions below are about the literals. To make this submission perfect, I would suggest to use doubles (in harmonic
you have an int
). You can choose which notation do you prefer (0.0
vs. 0.
vs. 0d
vs. 0.d
, vs. 0D
...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@siriak please have a look at the things mentioned above (like new dependencies).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Changes for feature request #4338