5
\$\begingroup\$

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:

enter image description here

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.

\$\endgroup\$
  • \$\begingroup\$ Please be aware that we review the code you posted, not the code you had in mind. We encourage you to post your real code. If you simplify the code that you post, you take the risk of receiving answers that are not fully relevant to you. \$\endgroup\$ – 200_success Jul 29 '16 at 8:08
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ First off, that is some sweet code and STL use that I'll burn in my memory now. I'd go with that were I trying to do exactly what I've implemented. I've edited my question to include some more information. Basically, I'd like to be able to extend my class to be able to tackle many different problems with the same signature. It seems that inheritance would very naturally solve this problem. I want to keep the data and these other objects as loosely coupled as possible, though, so the set of Statistics (or Reporter) implementations don't break if details of Stage are changed. \$\endgroup\$ – Salmonstrikes Jul 29 '16 at 0:52
  • 1
    \$\begingroup\$ @Salmonstrikes: I kind of took that for granted--but I'm not at all sure that in this case, there's likely to be any circumstance under which your Stage, Statistics, etc., are likely to save you much (if anything). With the code above, you mostly just change the lambda and possibly switch from accumulate to for_each. Either way almost guaranteed easier than using inheritance. \$\endgroup\$ – Jerry Coffin Jul 29 '16 at 1:02
  • \$\begingroup\$ Oh, I see. So the lambdas deal with the problem directly. In a complicated problem, I guess this means that may main() function could become pretty crowded (?), unless I just break up the problem into functions, C-style. So I'd have a library of functions instead of a library of classes for code reuse? \$\endgroup\$ – Salmonstrikes Jul 29 '16 at 1:39
  • 1
    \$\begingroup\$ @Salmonstrikes: Depending on what you're computing, it might make sense to explicitly define a class for a particular computation. It might even make sense to use a hierarchy of them--but if you do, a useful base class probably needs to define more than the fact that descendants operate on two arrays. \$\endgroup\$ – Jerry Coffin Jul 29 '16 at 15:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.