Mode and standard deviation for Math Library

Question

Taking forward the code written for the math library previously mentioned here. link. Wrote templatized functions for mode and standard deviations. (Some of the changes to the previous reviews were not made yet. Please review the same.

NB: I get certain comments regarding the inclusion of headers. This is a part of a visual studio project and some of the headers are moved to pch.h.

CODE

#include <vector>
#include <numeric>
#include <string>
#include <functional>
#include <unordered_map>

namespace Statistics
{
    template <typename T>
    T average(const std::vector<T> &distributionVector)
    {
        if (distributionVector.size() == 0)
        {
            throw std::invalid_argument("Statistics::average - The distribution provided is empty");
        }
        return std::accumulate(distributionVector.begin(), distributionVector.end(), T()) 
            / (distributionVector.size());
    }

    template <typename T>
    T variance(const std::vector<T> &distributionVector)
    {
        if (distributionVector.size() == 0)
        {
            throw std::invalid_argument("Statistics::expectation - The distribution provided is empty");
        }
        T meanOfSquare = average(distributionVector);
        return (std::accumulate(distributionVector.begin(), distributionVector.end(), T(), [=](T a,T b) { return a + (b - meanOfSquare )*(b - meanOfSquare); })/distributionVector.size());
    }

    template <typename T>
    T standardDeviation(const std::vector<T>& distributionVector)
    {
        return pow(variance(distributionVector), 0.5);
    }

    template<typename T>
    T mode(const std::vector<T>& distributionVector)
    {
        std::unordered_map<T, int> frequencyMap;
        std::for_each(distributionVector.begin(), distributionVector.end(), [&](T a) { frequencyMap[a]++;  });
        int maxCount = 0;
        std::for_each(frequencyMap.begin(), frequencyMap.end(), [&](auto a) { maxCount = std::max(maxCount, a.second); });
        T answer;
        std::for_each(frequencyMap.begin(), frequencyMap.end(), [&](auto a) { if (maxCount == a.second) { answer = a.first; } });
        return answer;
    }
}

Test code

#include "pch.h"
#include <vector>
#include "../MathLibrary/Combinatorics.h"
#include "../MathLibrary/Statistics.h"

void compareDoubles(double a, double b)
{
    const double THRESHOLD = 0.01;
    ASSERT_TRUE(abs(a - b) < THRESHOLD);
}

TEST(Combinatorial_Factorial, small_ints)
{
    EXPECT_EQ(Combinatorics::factorial(0), 1);
    EXPECT_EQ(Combinatorics::factorial(1), 1);
    EXPECT_EQ(Combinatorics::factorial(5), 120);
    EXPECT_EQ(Combinatorics::factorial(20), 2432902008176640000);
}

TEST(Combinatorial_Factorial, too_big)
{
    EXPECT_THROW(Combinatorics::factorial(500), std::invalid_argument);
}

TEST(Combinatorial_Combinations, small_ints)
{
    EXPECT_EQ(Combinatorics::combinations(5,5), 1);
    EXPECT_EQ(Combinatorics::combinations(5, 0), 1);
    EXPECT_EQ(Combinatorics::combinations(5, 1), 5);
    EXPECT_EQ(Combinatorics::combinations(20,10),184756);
    EXPECT_EQ(Combinatorics::combinations(40, 35),658008);
}

TEST(Combinatorial_Combinations, negative_n)
{
    EXPECT_THROW(Combinatorics::combinations(-5, 5), std::invalid_argument);
}

TEST(Combinatorial_Combinations, r_greater_than_n)
{
    EXPECT_THROW(Combinatorics::combinations(4, 5), std::invalid_argument);
}

TEST(Combinatorial_Combinations, overflow)
{
    EXPECT_THROW(Combinatorics::combinations(100, 50), std::invalid_argument);
}

TEST(Combinatorial_Permutations, small_ints)
{
    EXPECT_EQ(Combinatorics::permutations(5, 5), 120);
    EXPECT_EQ(Combinatorics::permutations(5, 0), 1);
    EXPECT_EQ(Combinatorics::permutations(5, 2), 20);
    EXPECT_EQ(Combinatorics::permutations(10, 2), 90);
    EXPECT_EQ(Combinatorics::permutations(40, 3), 59280);
    EXPECT_EQ(Combinatorics::permutations(40, 7), 93963542400);
    EXPECT_EQ(Combinatorics::permutations(50, 4), 5527200);
}

TEST(Combinatorial_Permutations, r_negative)
{
    EXPECT_THROW(Combinatorics::permutations(5, -5), std::invalid_argument);
}

TEST(Combinatorial_Permutations, n_negative)
{
    EXPECT_THROW(Combinatorics::permutations(-5, 5), std::invalid_argument);
}

TEST(Combinatorial_Permutations,r_greater)
{
    EXPECT_THROW(Combinatorics::permutations(5, 6), std::invalid_argument);
}

TEST(Combinatorial_Permutations,overflow)
{
    EXPECT_THROW(Combinatorics::permutations(50,46), std::invalid_argument);
}

TEST(Statistics_mean, small_distributions)
{
    std::vector<int> testVector = { -2,-1,0,1,2 };
    EXPECT_EQ(Statistics::average(testVector), 0);
    std::vector<double> testVectorDouble = {5,5,6,6};
    compareDoubles(Statistics::average(testVectorDouble), 5.5);
}

TEST(Statistics_mean, empty_distribution)
{
    std::vector<int> testVector;
    EXPECT_THROW(Statistics::average(testVector), std::invalid_argument);
}

TEST(Statistics_variance, small_distribution)
{
    std::vector<double> testVector = { 0,0 };
    compareDoubles(Statistics::variance(testVector), 0);
    std::vector<double> testVector2 = {1,2,3,4};
    compareDoubles(Statistics::variance(testVector2), 1.25);
    std::vector<double> testVectorRandom = { 1,2,3,4,6,8,9,34,45,78,89 };
    compareDoubles(Statistics::variance(testVectorRandom), 938.2314);
}

TEST(Statistics_standarddev, small_distribution)
{
    std::vector<double> testVector = { 0,0 };
    compareDoubles(Statistics::standardDeviation(testVector), 0);
    std::vector<double> testVector2 = { 1,2,3,4 };
    compareDoubles(Statistics::standardDeviation(testVector2), 1.11803);
    std::vector<double> testVectorRandom = { 1,2,3,4,6,8,9,34,45,78,89 };
    compareDoubles(Statistics::standardDeviation(testVectorRandom), 30.6305);
}

TEST(Statistics_mode, small_distribution)
{
    std::vector<int> testVector = { 32,32, 45, 12,32};
    EXPECT_EQ(Statistics::mode(testVector), 32);
    std::vector<int> testVector1 = { 32,32,32 };
    EXPECT_EQ(Statistics::mode(testVector1), 32);
    std::vector<int> testVector2 = {0};
    EXPECT_EQ(Statistics::mode(testVector2), 0);
}

Answer 1

       std::for_each(distributionVector.begin(), distributionVector.end(), [&](T a) { frequencyMap[a]++;  });

Use prefix ++ not postfix. If T is not an elementary integer type, the general implementation of postfix is to copy the argument and call the prefix version then return the argument. Get in the habit of using ++x not x++ in general.

        int maxCount = 0;
        std::for_each(frequencyMap.begin(), frequencyMap.end(), [&](auto a) { maxCount = std::max(maxCount, a.second); });
        T answer;
        std::for_each(frequencyMap.begin(), frequencyMap.end(), [&](auto a) { if (maxCount == a.second) { answer = a.first; } });

You make two passes why? The first finds the largest count, and the second then finds the last element having that count. You could simply remember the max and the associated value in the first loop. Another flaw of the second loop is not returning as soon as it finds the match, but runs through the whole collection anyway.

You can just use the max_element passing in a lambda for the comparison that compares the .second. It returns the entire pair, of which you return the .first.

More generally, the built-in range-based for loop is simpler and more flexible than the (original, begin/end iterator based) for_each algorithm.

Answer 2

        throw std::invalid_argument("Statistics::average - The distribution provided is empty");

Oops - we didn't include <stdexcept>.

    return pow(variance(distributionVector), 0.5);

What's pow()? If we meant std::pow(), we need to include <cmath>. And std::sqrt() would be clearer and more idiomatic than std::pow( , 0.5).

---

Now we have the code compiling, let's have a proper look inside.

template <typename T>
T average(const std::vector<T> &distributionVector)

I think a previous review advised that T might be a poor choice of return type. For example, we probably want a double for the mean of a vector of int. And accepting only vectors, rather than any range, is too inflexible for many users.

---

    int maxCount = 0;
    std::for_each(frequencyMap.begin(), frequencyMap.end(), [&](auto a) { maxCount = std::max(maxCount, a.second); });
    T answer;
    std::for_each(frequencyMap.begin(), frequencyMap.end(), [&](auto a) { if (maxCount == a.second) { answer = a.first; } });

Here, we're traversing the vector twice. We should just use std::max_element() to traverse the list once, then return the value from that element.

---

What's the mode of an empty list? It's surprising and inconsistent that this function doesn't throw when the list is empty, given that the other functions do.