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