Class Design: Calculating statistics from weighted values

Question

I was wondering if this is the best possible design for my situation. Note that this is a simpler version of the problem I am trying to tackle.

I have a class Stage that holds data as two vectors: weights w_ and values x_. I'm interested in calculating certain statistics like the weighted mean / variance etc. So I have a method Stage::ReportStatistic.

The Statistic class is abstract and goes as a pointer argument to Stage::ReportStatistic. The Statistic class has a method Statistic::Value that takes two vectors and calculates the statistic in any particular implementation, such as my StatisticMean::Value.

I find that my Statistic::Value method needs references to private members of the Stage class. Is there a way to avoid the signature Statistic::Value(std::vector<double> const &, std::vector<double> const &)? If I changed the representation of data in stage to std::vector<std::pair<double, double> > instead, it would break Statistic::Value. Is it asking for too much to come up with a design that could avoid this problem?

The code is complete and can be compiled using something like g++ -std=c++11 <whatever>.cpp.

#include <vector>
#include <memory>
#include <random>
#include <queue>
#include <stdexcept>
#include <iostream>

// BEGIN statistic.h
namespace project
{
    // Abstract class that takes in data and spits out a statistic through its Value() method
    class Statistic
    {
        public:
            virtual double Value(std::vector<double> const &, std::vector<double> const &) = 0;
    };
}
// END statistic.h

//BEGIN statisticmean.h
namespace project
{
    // An implementation of statistic whose Value() method computes the weighted mean of the data
    class StatisticMean: public Statistic
    {
        public:
            class StatisticMeanBuilder;
            StatisticMean() {}
            double Value(std::vector<double> const &x, std::vector<double> const &w);
    };

    class StatisticMean::StatisticMeanBuilder
    {
        public:
            StatisticMeanBuilder() {}
            std::unique_ptr<Statistic> Build()
            {
                std::unique_ptr<Statistic> x(new StatisticMean());
                return x; 
            }
    };
}
// END statisticmean.h

// BEGIN stage.h
namespace project
{
    // A data-holder. Method ReportStatistic returns a statistic according to the input.
    class Stage
    {
        private:
            std::vector<double> x_;
            std::vector<double> w_;
        public:
            Stage(std::vector<double> const &, std::mt19937_64 &);
            double ReportStatistic(std::unique_ptr<Statistic>);
    };
}
// END stage.h

// BEGIN statisticmean.cpp
namespace project
{
    // Implementation: nothing to see here...
    double StatisticMean::Value(std::vector<double> const &x, std::vector<double> const &w)
    {
        const int N = x.size();
        try
        {
            if(N == 0) throw std::logic_error("Error: Must have at least one x element in StatisticMean");
            if(w.size() != N) throw std::logic_error("Error: Vector sizes inconsistent in StatisticMean");
        }
        catch(std::logic_error &e)
        {
            std::cerr << e.what() << std::endl;
        }

        double result = 0.0;
        for(int i = 0; i < N; i++)
            result += x[i] * w[i];

        return result;
    }
}
// END statisticmean.cpp

// BEGIN stage.cpp
namespace project
{
    // Implementation: nothing to see here...
    Stage::Stage(std::vector<double> const &w, std::mt19937_64 &g)
    {
        int N = w.size();
        std::normal_distribution<double> du(0.0, 1.0);

        this->x_.resize(N);
        this->w_ = w;
        for(int i = 0; i < N; i++)
            this->x_[i] = du(g);
    }

    double Stage::ReportStatistic(std::unique_ptr<Statistic> s)
    {
        return s->Value(this->x_, this->w_);
    }
}
// END stage.cpp

// BEGIN main.cpp
int main()
{
    // Random number generator
    std::random_device rd;
    std::mt19937_64 g(rd());

    // Tiny vectors for working example
    std::vector<double> weights(3);
    weights[0] = 0.0; weights[1] = 0.0; weights[2] = 0.0;

    // Construct the data-holder
    project::Stage x(weights, g);

    // Create a list of statistics that you'd like to see
    // There's only a mean implementation, but you could imagine others (variance etc)
    std::queue<std::unique_ptr<project::Statistic> > s;
    s.push(std::move(project::StatisticMean::StatisticMeanBuilder().Build())); 

    while(!s.empty())
    {
        // Calculate each statistic according to the queue
        std::cout << x.ReportStatistic(std::move(s.front())) << std::endl;
        s.pop();
    }

    return 0;
}
// END main.cpp

A more realistic version of this problem:

Roughly speaking, this class takes probability distribution, samples from it, and then transforms each sample according to some model function. Once the object is constructed in this way, I'd like to report statistics on the transformed samples (say x here).

Anything pointers going into the methods of Stage would need to access the private members of Stage through something like references. If I change the representation of private data in Stage, it would force me to rewrite even the headers of Model and Reporter.

The pros of this design is that I can write thousands of Model and Reporter children according to what I want by simple inheritance. And it would work just fine unless, somewhere down the line, I needed to change how the data is represented in Stage. I'd like to avoid this problem.

Answer 1

Hmmm....let's consider what the standard library provides to do the same job you've done. Your weighted mean ends up being the same algorithm as what the standard library calls std::inner_product. Using the standard library roughly as (I think) it was intended to be used, we could end up with code something like this:

#define do_throw(f, l, s) throw std::logic_error("Error in " #f " at line " #l ": " s)
#define thrower(f, l, s) do_throw(f, l, s)

#define assure(b, s) ((b) || (thrower(__FILE__, __LINE__, s), 1))

int main()
{
    static const int N = 10000;

    std::random_device rd;
    std::mt19937_64 g(rd());

    std::uniform_real_distribution<double> d(0.0, 1.0);
    std::normal_distribution<double> du(0.0, 1.0);

    std::vector<double> weights;
    std::vector<double> values;

    try {
        std::generate_n(std::back_inserter(weights), N, [&] { return d(g); });
        std::generate_n(std::back_inserter(values), N, [&] { return du(g); });

        assure(weights.size() != 0, "Size == 0");
        assure(weights.size() == values.size(), "Size mismatch");

        std::cout << inner_product(weights.begin(), weights.end(),
            values.begin(), 0.0);
    }
    catch (std::exception &e) {
        std::cerr << e.what() << "\n";
        return 0;
    }
}

It doesn't seem to me that what you've invented adds enough new to that to justify itself.

If I were going to write something different from that, I'd probably start from the data. I think the idea of having a vector of pairs is a reasonable enough one that it's worth at least considering what code using that could look like. I'd think of something like this:

int main()
{
    static const int N = 3;

    std::random_device rd;
    std::mt19937_64 g(rd());

    std::uniform_real_distribution<double> d(0.0, 1.0);
    std::normal_distribution<double> du(0.0, 1.0);

    std::vector<std::pair<double, double>> vals;

    try {
        std::generate_n(std::back_inserter(vals), 
                        N, 
                        [&] { return std::make_pair(d(g), du(g)); });

        std::cout << std::accumulate(vals.begin(), vals.end(),
            0.0,
            [](double a, std::pair<double, double> const &b) { 
                return a + b.first * b.second; 
            });

    }
    catch (std::exception &e) {
        std::cerr << e.what() << "\n";
        return 0;
    }
}

The usual hope with an inheritance-based hierarchy is that it allows substantially improved flexibility and/or ease of implementation, at the expense of only minimal run-time overhead.

In this case, you seem to have achieved the minimal run-time overhead (only one virtual call per entire pair of arrays), but I don't see where you've accomplished much (if anything) in the way of adding flexibility or made implementation any easier at all (rather the opposite: you've actually added some overhead to implementation as well).

At least as I see it, the problem here is fairly simple: inheritance almost always adds a little bit of overhead:

1. You (obviously) have to inherit from the right base class.
2. You have to manipulate things via pointers.

In this case, the sum total of "knowledge" embodied in the base class is "we doing something with a couple of arrays". That doesn't impart enough useful information to the derived class that it simplifies implementation enough to make up for the normal overhead of using derivation to start with, so it ends up as a net loss. Worse, although we probably could embody a little more into the base class, I'm pretty sure even when we move as much there as we can, it still ends up a net loss.