Closing Issue/67

[nwolek]

@tap - Working with this algorithm during testing gave me some insight into it's limitations. I am pretty confident that it is working as expected, it is just quirky because of the way output history is incorporated.

I tried to document these insights in my brief Doxygen comments. Can you read these in particular and see if they make sense?

Oh, just noticed that I didn't use camelCase for last_out! Feel free to change if you want.

[tap]

to be consistent with other places in our code, should it be x0 (current input) and x1 (previous input) ?

[tap]

Hi @nwolek -- can you please review my comments in the code review above? Do they square with reality?

[nwolek]

Regarding x0,x1,x2,x3: when an interpolation requires 4 samples, that happens between x1 and x2. We now have some 2 sample algorithms that are overloaded to accept 4 samples. In this case, I made sure that the interpolation still happens between x1 and x2 so that it would remain consistent should we provide switching between algorithms in the future.

After this decision, I was faced with what to do in the two sample operators. I thought it would be better to remain consistent, so that through out our implementations, the interpolations always happen between x1 and x2. That way you have one set of comments at the start of the class that fits both operators (2 and 4 sample versions).

That was my logic, but I am happy to discuss if you disagree and think it makes things more confusing.

[tap]

Thanks @nwolek -- that sounds a bit tortured :-). Maybe we should think about this whole overloading business... I can't help but think that there is something elegant hiding from us.

(thinking... thinking... thinking...)

I don't know if this is a great idea or a terrible one, but I'm imagining something where we pass C++ iterators as the arguments. By doing this all functions would look something like this pseudo-code:

constexpr T operator()(double delta, std::vector<T>::const_iterator begin, std::vector<T>::const_iterator end) noexcept {
    // so we have a range within a vector now between begin and end
    // that might be 2 samples or 4 or 8, etc...
    // but we should be able figure that out and do the right thing...
}

I'm not sure if this would solve all our ills or just create new ones, but it make sense to explore this further. but first we should take a break!

[nwolek]

I have opened a new issue to discuss this redesign (issue #78). Since that conversation could take a while, I suggest we complete this pull request.

What is the best way to change the last_out variable to a private mY1? I am going to see if I can add it to the branch.

[nwolek]

That works. Another reason to make sure that pull requests happen from a terminal branch.

[tap]

Good plan to separate into a separate issue.