Re: [boost] Time Series "review"

From: Eric Niebler (eric_at_[hidden])
Date: 2007-08-07 23:35:02

Lewis Hyatt wrote:
> 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.

I wouldn't mind it, either. :-) If you're so motivated, You could polish 
the rolling average implementation I sent around not long ago.

> * 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 think there is a growing consensus that the named discretization 
classes should go.

> -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.

That's correct, dense_series only allows integral offsets. And you're 
also correct that the one-sentence summary of the time series library is 
not inclusive of series with floating-point offsets. The docs need 
updating, and the notion of a time series with floating point offsets 
needs a bit of a tweak, too.

> -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. 

But it's not an error that is likely to go unnoticed, IMO. "Hey, where'd 
all my data go? Oh, I forgot to call commit()." :-)

> 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 don't like the idea of commit() called automatically, and I also don't 
like the idea of needing to keep a reference count to know when to call it.

> -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.

That's true, the word "invert" has a couple of meanings here. Perhaps 
"reciprocal()"?

> * 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.

Thanks for taking the time.

-- 
Eric Niebler
Boost Consulting
www.boost-consulting.com
The Astoria Seminar ==> http://www.astoriaseminar.com