[boost] Time Series "review"

From: Lewis Hyatt (lhyatt_at_[hidden])
Date: 2007-08-07 16:46:55

Hi, here are my quick thoughts on this library. I don't have time for 
much more than this... sorry it isn't more thorough.

* What is your evaluation of the design?

        I guess my first impression was "way too many concepts", but after a 
while it all starts to make sense. It seems pretty flexible what you can 
do with it. It looks quite daunting at first to write new series 
classes, but that probably isn't going to be that common a task. Writing 
new algorithms seems pretty straightforward and I wouldn't mind trying 
to add some new ones.

* What is your evaluation of the potential usefulness of the library?

        I don't have a current use for it, but I can see how a lot of people would!

     * Did you try to use the library?  With what compiler?  Did you
have any problems?

        cygwin only has boost 1.33 still, and I didn't have time to get 1.34.1, 
so I was missing too much stuff to compile the library.
        
     * How much effort did you put into your evaluation? A glance? A
quick reading? In-depth study?

        About an hour reading the docs and looking at the examples.

     * Are you knowledgeable about the problem domain?

        More or less. I do a lot of data analyis with std::vector and legacy C 
libraries like clapack.

Here are some random comments:

-I don't like the names "daily", "weekly", etc, because they give the 
impression that the library is only meant for daily data. I think it 
would be better just to define a template stride<> inheriting from 
mpl::int_<>, so you could write instead something like

        dense_series<double, stride<7> >

and the meaning of 7 would depend on the context of the program.

-I am confused by the statement in the docs that "A time series is a 
series of data points, sampled at regular intervals". If that's true, 
then what does it mean to use a floating point offset? Doesn't that mean 
I can use irregular intervals too? I was thinking it would be natural to 
have an FFT algorithm for a dense time series, but that would only make 
sense if it was sampled at regular intervals. I guess a dense series 
always has an integral offset type, so it would make sense there.

-I worry a little about the dual interfaces for ordered_inserter, maybe 
it should be split into two classes? Especially for the output iterator 
usage, I think people would frequently try to use it with std::copy and 
then forget to call commit(), causing silent errors. Maybe that's just 
something the user will have to get used to. Another option could be to 
use a new class called ordered_insert_iterator with a shared_ptr pImpl 
setup, so that the last temporary object would call commit() from its 
destructor after the algorithm completed. In that case, problems would 
be caused if people created non-temporary objects, but I think in 
practice, in the majority of cases, the output iterator would be used by 
creating a temporary object and passing it to std::copy or whatever.

-I think invert_elements() should have a different name. For one thing, 
there is confusion with invert_heaviside(), which is completely 
unrelated, and for another, this name could also work fine to describe a 
function which multiplied every element by -1. I can't think of a decent 
one, though.

* Do you think the library should be accepted as a Boost library?

Given that I didn't have time to look into this much more, I'm not sure 
how seriously my opinion can be taken, but I do vote yes.

-Lewis