2
\$\begingroup\$

Let's say we have the following class which performs interpolation on irregularly spaced data:

template<class Real>
class interpolator
{
public:
    interpolator(const Real* const x, const Real* const y, size_t length)
    {
        m_x.resize(length);
        m_y.resize(length);
        for (size_t i = 0; i < length; ++i)
        {
             m_x[i] = x[i];
             m_y[i] = y[i];
        }
    }

    Real interpolate_at(Real x)
    {
        // use m_x and m_y to produce an estimate of y(x):
        return something;
    }

private:
    std::vector<Real> m_x;
    std::vector<Real> m_y;
};

This design has a number of things going for it: For one it is easy to use:

std::vector<double> x;
std::vector<double> y;
// Initialize x and y ...
interpolator<double> interp(x.data(), y.data(), y.size());

Since the constructor accepts primitive types, it can also be wrapped in Python with moderate effort (you'd need a shared object and some template instantiations, but let's assume that can be done). You can also let x and y go out of scope and the interpolator will still work.

The tragic flaw is that it performs a huge memcpy, which is slow and not space efficient.

Is there a way to refactor this class such that

  • It is easy to wrap in Python (std::shared_ptr's in interfaces are a nightmare to wrap . . .)
  • It doesn't perform a memcpy
  • The input data can be allowed to go out of scope and the interpolator still work
\$\endgroup\$
  • 2
    \$\begingroup\$ I don't think this can really be reviewed as it is now. For example, your interpolate_at is purely a stub that won't even compile (much less actually accomplish anything). \$\endgroup\$ – Jerry Coffin Mar 14 '17 at 16:38
  • 1
    \$\begingroup\$ As a rule, "Let's say we have…" questions are too hypothetical to review. \$\endgroup\$ – 200_success Mar 14 '17 at 16:47
  • \$\begingroup\$ Uhm. I've read that too late. I found this example already lacking enough. \$\endgroup\$ – Maikel Mar 14 '17 at 16:54
  • \$\begingroup\$ The input data can be allowed to go out of scope and the interpolator still work you can't really escape this without copying the data. If copying is too expensive, keep the input in scope long enough to safely perform any calculation. \$\endgroup\$ – D. Jurcau Mar 14 '17 at 16:56
  • 1
    \$\begingroup\$ @D.Jurcau Technically, this does not mean, that the data is not allowed to be moved to interpolator. \$\endgroup\$ – Maikel Mar 14 '17 at 17:07
1
\$\begingroup\$

Don't pass arrays as pointers ...

... for multiple reasons. Your current design

  1. always copies the arrays x, y deeply
  2. can be misused by passing nullptr and trusts the user that length is chosen correctly.

Instead of

interpolator(const Real* const x, const Real* const y, size_t length) {  /* ... */ }

use

interpolator(std::vector<Real> x, std::vector<Real> y)
: m_x{std::move(x)}
, m_y{std::move(y)}
{ if (m_x.size() != m_y.size()) throw std::logic_error{"NO!"}; }

or if the memory layout doesn't effect the performance too much

interpolate(std::vector<std::array<Real, 2>> xy): m_xy{std::move(xy)} {}

If you want to initialise your data from any range use templates and constrain its type to guard for errors at compile time.

Prefer regular functions for computations

This means, that an implemented function is also a function in the mathematical sense. Same input data produces same output data. I found that this almost always leads to evaluations which do not change an inner state. Thus it smells to me that

Real interpolate_at(Real);

is not

Real interpolate_at(Real) const;
\$\endgroup\$

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