2D Histogram class

Question

I scraped some data looking at trajectories of popular posts on Reddit, and now I want to group and histogram them by type. I put this class together in C++ to do so. Characteristics of the data are that they contain a value and a time (treated as y value and x value) for each data point and that the number of data points varies per post. So I designed a class that creates 2D histograms plotting value against time in a number of ways. I would like feedback on everything from style to efficiency to code readability.

Hist2D.h

#ifndef HIST_2D
#define HIST_2D

#include "ListOfNumericLists.h"
#include "NumericList.h"
#include <ostream>

template <typename T>
class Hist2D
{
public:
  //histogram can be aligned in a variety of ways
  enum class Alignment {
    Front, Back, AtMax, ByX
      };

  //the defaults for Hist2D constructor are based on x varying from 0 to 23 hours and y varying from 0 to 25 (ranking, predetermined range from webscraping parameters)
  Hist2D(int xBins=26, int yBins=28, double xMin = -.1, double xMax = 24, double yMin = -.1, double yMax = 25.1) :
    m_xBins(xBins), m_yBins(yBins),  m_xMin(xMin), m_xMax(xMax), m_yMin(yMin), m_yMax(yMax), m_xVals(xBins+1), m_yVals(yBins+1),  m_matrixCount(boost::extents[yBins][xBins])
  {
    //all bins start counting from zero
    std::fill(m_matrixCount.origin(), m_matrixCount.origin() + m_matrixCount.size(), 0);

    //calculate binning increments for x and y to include all values between specified min and max, binned in equally sized and uniformly distributed ranges
    double xInc = (xMax - xMin)/xBins;
    double yInc = (yMax - yMin)/yBins;
    for(int i = 0; i<(xBins+1); i++){ m_xVals[i] = xMin + i*xInc; }
    for(int i = 0; i<(yBins+1); i++){ m_yVals[i] = yMin + i*yInc; }
  }       

  void print(std::ostream& os) const {

    int width = m_matrixCount.shape()[0];
    int height = m_matrixCount.shape()[1];

    for(int i = 0; i < (width-1); i++){
      for(int j = 0; j < height; j++){
        os << " " << m_matrixCount[i][j] << ",  ";
      }
      //separate out last column of each line to avoid stray ','
      os << " " << m_matrixCount[i][width-1];
      os << std::endl;
    }

  }

  //adds many series to histogram passed in through a container class, ListOfNumericLists
  void addToHist(const ListOfNumericLists<T>& listOfLists, Alignment alignment ){
    auto list = listOfLists.getList();
    for(auto it = list.begin(); it!=list.end(); ++it){
      addToHist(*it, alignment);
    }
  }

  //adds singe series to histogram
  void addToHist(const NumericList<T>& numList, Alignment alignment) {
    const std::vector<T>& toAdd = numList.getNumConst();
    int i, xInd, yInd;
    switch(alignment){
    //use this method to align all series at their beginning
    //this means the 'x' value will be ignored
    case Alignment::Front :
      {
    i = 0;
    for (auto it = toAdd.begin(); it != toAdd.end(); ++it) {
      xInd = std::lower_bound(m_xVals.begin(), m_xVals.end(), i) - m_xVals.begin() - 1;
      yInd = std::lower_bound(m_yVals.begin(), m_yVals.end(), *it) - m_yVals.begin() - 1;
      m_matrixCount[yInd][xInd] +=1;
      i++;
    }
    break;
      }
    //use this method to align all series at their end
    //this means the 'x' value will be ignored
    case Alignment::Back :
      {
          i = m_matrixCount.shape()[0] - 1;
          for (auto it = toAdd.rbegin(); it != toAdd.rend(); ++it) {
            xInd = std::lower_bound(m_xVals.begin(), m_xVals.end(), i) - m_xVals.begin() - 1;
            yInd = std::lower_bound(m_yVals.begin(), m_yVals.end(), *it) - m_yVals.begin() - 1;
            m_matrixCount[yInd][xInd] +=1;
            i--;
          }
          break;
      }
    //use this method to align all series at their 'max' (here it is the 'min' because rank starts low and goes high)
    //this means the 'x' value will be ignored
    case Alignment::AtMax :
      {
          i = m_matrixCount.shape()[0]/2;
          auto it = std::find(toAdd.begin(), toAdd.end(), numList.m_minVal);
          auto minIndex = std::distance(toAdd.begin(), it);
          i -= minIndex;
          for (auto it = toAdd.begin(); it != toAdd.end(); ++it) {
            xInd = std::lower_bound(m_xVals.begin(), m_xVals.end(), i) - m_xVals.begin() - 1;
            yInd = std::lower_bound(m_yVals.begin(), m_yVals.end(), *it) - m_yVals.begin() - 1;
            m_matrixCount[yInd][xInd] +=1;
            i++;
          }
          break;
            }
    //use this method to align all series according to their 'x' value
    //'x' is not ignored in this alignment method
    case Alignment::ByX :
      {
          const std::vector<T>& toAddX = numList.getXConst();
          for (unsigned int i = 0; i < toAddX.size(); i++) {
            xInd = std::lower_bound(m_xVals.begin(), m_xVals.end(), toAddX[i]) - m_xVals.begin() - 1;
            yInd = std::lower_bound(m_yVals.begin(), m_yVals.end(), toAdd[i]) - m_yVals.begin() - 1;
            m_matrixCount[yInd][xInd] +=1;
          }
          break;
      }
    default:
      std::cout << "Improper alignment parameter in Hist2D addToHist method" << std::endl;
    }
}


private:
  typedef boost::multi_array<T, 2> array_type;
  int m_xBins;
  int m_yBins;
  double m_xMin;
  double m_xMax;
  double m_yMin;
  double m_yMax;
  std::vector<double> m_xVals;
  std::vector<double> m_yVals;
  array_type m_matrixCount;

  //do not allow users to create an unsized histogram
  Hist2D() = delete;
};

#endif // HIST_2D

For any readers generous enough to comment on more extensive code, I am including ListOfNumericLists.h and NumericList.h below. I don't think it's necessary to look at these to critique what's above, but I'm sure are additional improvements.

The short summary:

• NumericList is a wrapper class for an std::vector of type T, expected to be numeric, along with an equal length second vector that should provide the x values for the original vector. The idea is to describe a series (time series or any other series where a value matches an x value of some kind). The list returns measures about the series, such as mean, standard deviation, number of unique values (since I originally wrote this for integers, where such a measure is meaningful). Also it counts inflection points, finds min, and max, and returns all these summary numbers in a single vector.
• ListOfNumericLists is a wrapper class for an std::vector containing NumericLists. It's useful if you want to concatenate all the lists to do broad summaries over an entire data set of individual series rather than looking at differences between series.

NumericList.h

#ifndef NUMERIC_LISTS
#define NUMERIC_LISTS

template <typename T>
class NumericList
{
 public:
  T m_maxVal;
  T m_minVal;
  T m_maxX;
  T m_minX;
  int length;

  NumericList()  {}
 NumericList(const std::vector<T>& newNumbers, const std::vector<T>& newX)
   : m_numbers(newNumbers), m_x_axis(newX)
  {
    m_maxVal = *max_element(m_numbers.begin(), m_numbers.end());
    m_minVal = *min_element(m_numbers.begin(), m_numbers.end());
    m_maxX = *max_element(m_x_axis.begin(), m_x_axis.end());
    m_minX = *min_element(m_x_axis.begin(), m_x_axis.end());
    length = m_numbers.size();
  }

  const std::vector <T>&  getNumConst() const { return this->m_numbers;};
  const std::vector <T>&  getXConst() const { return this->m_x_axis;};
  const std::vector <T>&  getNumUniqueConst() 
  {
    if(!uniqueComputed){
      computeUnique();
    }
    return m_uniqueNumbers;
  }
  const int getUniqueCount() 
  {
    if(!uniqueComputed){
      computeUnique();
    }
    int diversity = (int) m_uniqueNumbers.size();
    return diversity; 
  }
  const double getMeanUnique()
  {
    if(!uniqueComputed){
      computeUnique();
    }
    return  calcMean(false);
  }
  const double getSDUnique() {
    if(!uniqueComputed){
      computeUnique();
    }
    return calcSD(false);
  }
  const double getMean(){ return calcMean(true);}
  const double getSD(){ return calcSD(true);}

  //count the number of times the direction of the series changes (increasing to decreasing or vice versa)
  //vased only on the value and its position in the value array, not based on the 'x' values
  const int getInflectionCount(){
    if(!inflectionComputed){
      m_inflectionCount = 0;
      std::vector<T> m_inflection(m_numbers.size() - 1);
      std::adjacent_difference(++m_numbers.begin(), m_numbers.end(), m_inflection.begin());
      for(unsigned int i = 1; i<m_inflection.size(); i++){
    if( (i > 0) && ((m_inflection[i] <0) != (m_inflection[i-1]<0)))
      m_inflectionCount += 1;
      }      
      inflectionComputed = true;
    }
    return m_inflectionCount;
  }

  const std::vector<double> getAllData(){
    std::vector<double> result(9);
    result[0] = m_numbers.size();   
    auto min_max_value = std::minmax_element(m_numbers.begin(), m_numbers.end());    
    result[1] =  *(min_max_value.first); 
    result[2] = *(min_max_value.second);
    min_max_value = std::minmax_element(m_x_axis.begin(), m_x_axis.end());
    result[3] = *(min_max_value.first);
    result[4] = *(min_max_value.second);
    result[5] = getInflectionCount();
    result[6] = getUniqueCount();
    result[7] = calcMean();
    result[8] = calcSD();
    return result;
  }

  void print(std::ostream& os, const NumericList<T>& numList) const {
    auto numVec = numList.getNumConst();
    printVec(os, numVec);
  }

 private:
  std::vector<T> m_numbers = { };
  std::vector<T> m_x_axis = { };
  std::vector<T> m_uniqueNumbers = { };
  std::vector<T> m_inflection = { };
  int m_inflectionCount;
  bool uniqueComputed = false;
  bool inflectionComputed = false;

  void printVec(std::ostream& os, const std::vector<T>& v) {
    for(int i = 0; i<(v.size()-1); i++){
      os << v[i] << ", ";
    }
    os << v[v.size()-1];
    os << std::endl;
  }

  void computeUnique(bool useDefault = true){
    m_uniqueNumbers.assign(m_numbers.begin(), m_numbers.end());
    auto new_end = std::unique(m_uniqueNumbers.begin(), m_uniqueNumbers.end());
    m_uniqueNumbers.erase(new_end, m_uniqueNumbers.end());
  }

  //calculates the mean, of the series if useDefaul=true
  //if false, of the unique values in the series
  double calcMean(bool useDefault = true) const {
    //calculation for series
    if(useDefault){
      if (!m_numbers.empty()) {
    double sum  = std::accumulate(m_numbers.begin(), m_numbers.end(), 0.0);
    return  sum / m_numbers.size();
      } else {
    return -999.0;    }
    }
    //calculation for unique values in series
    else
      {
    if (!m_uniqueNumbers.empty()) {
      double sum  = std::accumulate(m_uniqueNumbers.begin(), m_uniqueNumbers.end(), 0.0);
      return  sum / m_uniqueNumbers.size();
    } else {
      return -999.0;    }     
      }
  }

  //calculates the standard deviation of the series  if useDefault=true
  //if false, of the unique values in the series
  double calcSD(bool useDefault = true) const {
    //calculation for all values in series
    if(useDefault){      
      if (!m_numbers.empty()) {
    double mean = calcMean(useDefault);
    double square_sum = 0.0;
    for(unsigned int i = 0; i < m_numbers.size(); i++){
      square_sum += (m_numbers[i] - mean) * (m_numbers[i] - mean);

    }
    square_sum /= m_numbers.size(); 
    return square_sum;
      } else {
    return -999.0;
      }
    }
    //calculation for unique values in series
    else
      {
    if (!m_uniqueNumbers.empty()) {
      double square_sum = std::inner_product(m_uniqueNumbers.begin(), m_uniqueNumbers.end(), m_uniqueNumbers.begin(), 0.0);
      double mean = calcMean(useDefault);
      return std::sqrt(square_sum/m_uniqueNumbers.size() - mean*mean);
    } else {
      return -999.0;
    }
      }
  }
};

#endif // NUMERIC_LISTS

ListOfNumericLists.h

#ifndef LIST_NUMERIC_LISTS
#define LIST_NUMERIC_LISTS

#include "NumericList.h"

template <typename T>
class ListOfNumericLists
{

 public:
 ListOfNumericLists()
   : was_concatenated(false)
    {};
 ListOfNumericLists(const std::vector<NumericList<T>>& list)
   : m_list(list), was_concatenated(false)
    {};

  void AddToList(const NumericList<T>& list){
    m_list.push_back(list);
    maxLength = std::max(maxLength, list.length);
    m_maxVal = std::max(m_maxVal, list.m_maxVal);
    m_minVal = std::min(m_minVal, list.m_minVal);
    m_maxVal = std::max(m_maxVal, list.m_maxVal);
    m_minVal = std::min(m_minVal, list.m_minVal);    
    if(was_concatenated){
      m_concatenated_list.insert(m_concatenated_list.begin(), list.begin(), list.end());
    }
  }
  void Concatenate(){
    if(!was_concatenated){
      std::vector<T> toConcat;
      std::vector<T> numHolder;
      for( auto it = m_list.begin(); it != m_list.end(); it++){
    numHolder = (*it).getNumConst();
    toConcat.insert(toConcat.end(), numHolder.begin(), numHolder.end());
      }
      m_concatenated_list = NumericList<T>(toConcat);
    } 
  }
  const NumericList<T>& GetConcatenated() const{
    return m_concatenated_list;
  }
  void print(std::ostream& os) const {
    for (auto it = m_list.begin(); it != m_list.end(); it++){
      (*it).print(os);
    }
  }

 private:
  std::vector<NumericList<T>> m_list;
  int maxLength;
  T m_maxVal;
  T m_minVal;
  T m_maxX;
  T m_minX; 
  NumericList<T> m_concatenated_list;
  bool was_concatenated; 
};

#endif // LIST_NUMERIC_LISTS

Answer 1

I see a number of things that may help you improve your code.

Use all of the required #includes

The NumericList.h file is missing a number of #includes required for its interface. They are:

#include <vector>
#include <iostream>
#include <numeric>
#include <algorithm>
#include <cmath>

Also, there is a similar dependency in Hist2D.h. It is missing one #include:

#include <boost/multi_array.hpp>

Place the const keyword appropriately

The NumericList.h file includes this line and others that are similar:

const double getMean(){ return calcMean(true);}

The problem with it is that it says that it's returning a const double which is not what you really meant. (If it is what you meant, you should be aware that the compiler will simply ignore type qualifiers on return types.) What you mean, I think, is that the underlying NumericList is unaltered by the getMean() call. To express that notion, the syntax would be this:

double getMean() const { return calcMean(true);}

Eliminate spurious semicolons

In a few places, including constructors for ListOfNumericLists, there are spurious semicolons:

ListOfNumericLists()
   : was_concatenated(false)
    {};  // <= this semicolon is neither needed nor wanted

Eliminating these will make the code more readable and understandable.

Understand the limits of default parameters

It's not necessarily wrong, but it's probably useful to point out that this part of Hist2D:

//do not allow users to create an unsized histogram
Hist2D() = delete;

seems to somewhat contradict this constructor

Hist2D(int xBins = 26, int yBins = 28, double xMin = -.1, double xMax = 24, double yMin = -.1, double yMax = 25.1): //etc.

The constructor has default arguments for every parameter, but using that constructor with no specified parameter values is explicitly prevented by the delete.

Add documentation and/or test function

It's not easy to understand the intended use or intended output of this class, so I can't comment further. Here's what I tried:

template <typename T>
NumericList<T> normalList(std::size_t len)
{
    static std::random_device rd;
    static std::mt19937 gen(rd());
    static std::normal_distribution<> dist(len/2,2);
    std::vector<T> x;
    std::vector<T> y;
    x.reserve(len);
    y.reserve(len);
    for ( ; len; --len) {
        x.push_back(100 + dist(gen));
        y.push_back(dist(gen));
    }
    return NumericList<T>(y, x);
}

int main()
{
    ListOfNumericLists<float> lists;
    Hist2D<float> hist{10, 10};
    hist.addToHist(normalList<float>(10), Hist2D<float>::Alignment::Front);
    hist.print(std::cout);
    std::cout << std::endl;
}

Sample output

 0,   1,   0,   1,   0,   0,   0,   0,   0,   0,   0
 0,   0,   1,   0,   0,   0,   0,   0,   0,   0,   0
 3,   1,   2,   1,   0,   0,   0,   0,   0,   0,   0
 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0
 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0
 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0
 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0
 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0
 0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0

Is that what was expected? Intended? It's not clear to me from the description or comments.