Skip to content

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

Merged
merged 11 commits into from
Sep 2, 2023
Merged

Added different types of Mean #4339

merged 11 commits into from
Sep 2, 2023

Conversation

punit324
Copy link
Contributor

@punit324 punit324 commented Aug 30, 2023

Changes for feature request #4338

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized it.
  • All filenames are in PascalCase.
  • All functions and variable names follow Java naming conventions.
  • All new algorithms have a URL in their comments that points to Wikipedia or other similar explanations.

@punit324 punit324 closed this Aug 30, 2023
@punit324 punit324 reopened this Aug 30, 2023
Copy link
Member

@vil02 vil02 left a 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 an IllegalArgumentException or use Optional as a return type.

  • naming: what do you think to rename this class to Means and/or shorten the method names to arithmetic, 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 as

    public 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 and java.util.Arrays.

@punit324
Copy link
Contributor Author

punit324 commented Aug 31, 2023

@vil02 I consider all your suggestion and will incorporate them.
Except one, where you are suggesting to use some iterable for input type other than double[].
As Mean can only be computed for numbers, double can hold all types of numbers including intergers and floats.
To consider your suggestion how about using List<Double> class as a input type.

@siriak siriak requested a review from vil02 August 31, 2023 15:22
@siriak
Copy link
Member

siriak commented Aug 31, 2023

@vil02 please review again

Copy link
Member

@vil02 vil02 left a 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[].

@punit324
Copy link
Contributor Author

punit324 commented Sep 1, 2023

@vil02
I've incorporated all your suggestions.
Please review again.

Copy link
Member

@vil02 vil02 left a 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?

Copy link
Member

@vil02 vil02 left a 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 ...).

Copy link
Member

@vil02 vil02 left a 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).

Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@siriak siriak enabled auto-merge (squash) September 2, 2023 17:20
@siriak siriak merged commit a96ad84 into TheAlgorithms:master Sep 2, 2023
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.

3 participants