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
> * 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
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk