A FIR filter using Modern C++ features

Question

I'm diving into "modern CPP", with wide usage of templates and containers, So I started with FIR filter.

In signal processing, a finite impulse response (FIR) filter is a filter whose impulse response (or response to any finite length input) is of finite duration, because it settles to zero in finite time. This is in contrast to infinite impulse response (IIR) filters, which may have internal feedback and may continue to respond indefinitely (usually decaying).

So I define FIR filter as a class and apply it to single scalar value or to the vector itself

Could you please review my code in terms of C++ coding?

#include <iostream>
#include <vector>
#include <list>
#include <algorithm>
#include <complex>
#include <cmath>
#include <iomanip>
#include <sstream>

// - Class method : operator(), constructor, various methods
// - Templates class and function
// - List and vector containers
// - Functors


template <typename T> std::string to_str(T & arr){
  std::ostringstream os;
  os << "[";  for (auto x: arr){    os<<" "<< x <<" ";  }  os << "]\n"; 
  return os.str();
}

template <class T> class FIR { 
public: 
    std::vector< T > coeff;  // Array of coefficents is static all the time
    std::list< T > taps;  // Array of is are shifting each tick
    int order;

    FIR(std::vector< T > v)  
    { 
       coeff = v; 
       order = coeff.size();
       taps = std::list< T > (coeff.size(), 0);
    } 
    void reset(){
       taps = std::list< T > (coeff.size(), 0);
    }
    void append( T  x){
      taps.push_front( x);
      taps.pop_back();
    }

    auto sum_product(){
      T  psum = 0;
      typename   std::list< T >::iterator t;
      typename std::vector< T >::iterator c;
      // Can we do better here?
      for (   c =coeff.begin(),     t  =taps.begin(); 
              c!=coeff.end(),       t !=taps.end();
            ++c             ,     ++t             ){
        psum += (*c) * (*t);
      }
      return psum;
    }
    
    auto operator() ( T  x){
      append(x);
       T  psum = sum_product();
      return psum;
    }

     auto operator() (std::vector< T > x){
     std::vector< T > y( x.size(), 0);
     for (int i = 0; i<x.size(); ++i){
        y[i] =  operator()(x[i]);
     }
     return y;
     }
}; 
  
int main() 
{ 
    std::vector<double> coeff = {0.0,1.0,5.0, 1.0, 0.0};
    
    FIR<double> fir(coeff); 
    std::cout <<"Coeff: " << to_str(fir.coeff);
    std::cout <<"Taps : "  << to_str(fir.taps);
    
    
    std::cout<<"\nTRANSFORM IN THE LOOP \n";
    std::vector<double> x_array = {0,1,2,3,4,5,6,0,0,0,0,0,0,13};
    for (auto x:x_array){
      auto p = fir(x);
      std::cout<<p<<"  ";
    }
    std::cout<<"\n";
    fir.reset();

    std::cout<<"\nTRANSFORM WITH FUNCTOR FOR SCALAR\n";
    std::vector<double> y_array( x_array.size(), 0);
    std::transform(x_array.begin(), x_array.end(), y_array.begin(), fir); 
    std::cout << to_str(y_array);
    fir.reset();

    std::cout<<"\nAPPLY FUNCTOR FOR CONTTAINER \n";
    auto y_2 = fir(x_array);
    std::cout << to_str(y_2);
    fir.reset();

    return 0; 
} 

Answer 1

Use member initializer lists in constructors where possible

Using member initializer lists can be more optimal and avoid issues when an error is thrown in a constructor. Use it where possible. For example:

FIR(const std::vector<T> &v): coeff(v), taps(coeff.size()), order(coeff.size()) {}

Make sure you initialize the members in the same order as you declared them.

Use const references when appropriate

Passing a vector by value means a complete copy has to be created. This wastes CPU time and memory. Use a const reference in that case, as I've done above.

Avoid storing redundant information

There is no need to store order, you can already get the same value by using coeff.size().

Make member variables const if they should never change

Since the coefficients never change after creating an instance of FIR, you can make coeff const.

Make functions that don't change any member variables const

Not only variables, but member functions themselves can be marked const. Do this when they don't change any member variables. This helps the compiler generate more optimal code, and will allow it to generate error messages if you accidentily do modify a member variable. For example:

auto sum_product() const {
    ...
}

Use a std::queue for taps

A std::list has \$\mathcal{O}(1)\$ insertion and removal at both ends, but the problem is that every element needs a separate heap allocation, pointers need to be kept up to date, and when looping over the elements in a list, they are not nicely layed out in memory. There is a better container for this: std::deque. However, since you use it as a queue, use the std::queue "container adapter" instead, so it is clear what the function of taps is.

If performance is really crucial, perhaps using a std::vector would be best, and use it as a circular buffer. But don't do that unless you really need it, because it will complicate your code.

Use STL algorithms where appropriate

The STL not only provides handy containers, but also algorithms that you can use, so you don't have to write them out yourself. For example, sum_product() can be replaced with a call to std::inner_product():

auto operator()(T  x) {
    append(x);
    return std::inner_product(coeff.begin(), coeff.end(), taps.begin());
}

Answer 2

• class FIR has nothing private. Should it be struct?

• The line

    std::list< T > taps;  // Array of is are shifting each tick
  

  is somewhat contradictory. It taps a list or an array?

• I don't see how order is used. Does it need to be a class member at all?

• operator()(T x) is very confusing. I would argue that it shall not be an operator at all; if you really want this to be an operator, consider operator<<. A metaphor of shifting into a filter is much more understandable than calling a filter.

• // Can we do better here? Yes.

Answer 3

  for (   c =coeff.begin(),     t  =taps.begin(); 
          c!=coeff.end(),       t !=taps.end();
        ++c             ,     ++t             ){
    psum += (*c) * (*t);
  }

I'm guessing the comma in the for loop condition should be an &&. It might be cleaner to use an index in this loop instead (especially if the two arrays are always the same size).