Re: [boost] [time_series] Review

From: Eric Niebler (eric_at_[hidden])
Date: 2007-08-08 01:01:09

Steven Watanabe wrote:
> AMDG
> 
> I still need to think a bit more before I vote but here
> are all the comments I have so far:
> 
> In the Series Interface section in the user's guide
> shouldn't
>     series-type( A0 const &, A2 const &, ... );
> be prefaced by
>     template<class A0, class A1, ...>
> ?

True, thanks.

> "It may seem odd that even the unit series have a value parameter..."
> Wouldn't it be better to use a traits template to
> determine what "one" should be and require that this template be 
> specialized?
> If there is no unambiguous unit value for a type
> then isn't it a little odd to use a unit series?

I don't think it's odd. The example I gave was when value_type is 
std::vector<int>. What is the unit value for this type? It could be 
std::vector<int>( N ,1 ) for any value of N.

> Discretization | Equivalent To
> daily          |   mpl::int_<1>
> ...
> These are guaranteed not to be just typedefs I hope?

Correct, they're their own types. But it's come up a lot, and I think 
it's best to just drop them.

> You should state that the runtime discretization compatibility check
> is an assertion as opposed to throwing an exception. You also need to
> specify exactly what it means for discretizations to be the same--that the
> discretizations must be the same type (Otherwise I might be inclined
> to consider 1l and 1 equal) and that operator== must be defined (
> Else I would likely try
>     struct my_discretization {};
>     typedef sparse_series<double, my_discretization> series_type;
> ).

The requirement that discretization are equality-comparable is specified 
as part of the TimeSeries concept:

http://boost-sandbox.sourceforge.net/libs/time_series/doc/html/TimeSeries.html

It would be nice to be more explicit about all this in the users' doc, 
though.

> Shouldn't ordered_inserter use the named parameters start, stop, and value?

A good idea.

> The promotion rules are complicated is there a metafunction that
> computes the result types or should I just use Boost.Typeof?

There is, but there isn't user documentation for it. In the 
time_series::traits namespace there are templates plus, minus, 
multiplies and divides which take and return storage tags types. And 
given a result storage tag type, you can generate a time series using 
traits::generate_series.

It's admittedly a bit complicated, and all the storage tag machinery is 
an undocumented detail. Do you need this functionality? I could consider 
simpler ways of getting at this information and documenting it.

> Is this specialization missing in traits/conversion.hpp
>     template<typename From, typename To>
>     struct is_storage_convertible<scaled_storage_tag<From>, To> : 
> is_storage_convertible<From, To> {};

No. "scaled_storage_tag" is a very bad name---it is unrelated to the 
scaled_series, which is probably where you got your idea. It is used to 
differentiate between normal series and their unit series variants. 
Those conversions only go one way.

> serialize should use nvp so that it will work with xml archives.

Noted.

> The following fails to compile because serialization is not defined for 
> daily:
> 
>     #include <iostream>
>     #include <boost/time_series/sparse_series.hpp>
>     #include <boost/archive/text_oarchive.hpp>
> 
>     typedef boost::time_series::sparse_series<double, 
> boost::time_series::daily> series_1;
> 
>     int main() {
>         series_1 s1;
>         boost::archive::text_oarchive ar(std::cout);
>         ar << s1;
>     }

Good catch.

> The comparison operators for time_series_base are defined
> in time_series_facade. Shouldn't they be in utility/time_series_facade.hpp?

Not really, but IMO they shouldn't be defined as they are. These 
functions require the series types to be derived from time_series_base. 
Rather, this should be a generalized template, conditionally disabled 
with enable_if<mpl::and_<is_time_series<Left>, is_time_series<Right>, 
...> or something.

> time_series_facade line 256:
>     template<typename Left, typename Right>
>     bool operator !=(time_series_base<Left> const &left, 
> time_series_base<Right> const &right)
>     {
>         return ! operator ==(left.cast(), right.cast());
>     }
> This rules out a member operator==.  Since you're explicitly
> downcasting and invoking ADL I would expect any definition
> of operator== to work.

Sure.

> time_series_facade.hpp line 280
>             this->sout_
>                 << " value = " << value << ", "
>                 << " offset = " << start << ", "
>                 << " end offset = " << stop << '\n';
> value shouldn't have a space before it should it?

Probably not.

> time_series_facade.hpp line 395
>     template<typename Derived, typename Storage, typename Discretization>
>     struct tag<time_series::time_series_facade<Derived, Storage, 
> Discretization> >
>     {
>         typedef time_series_tag type;
>     };
> Should this be typedef tag<Derived>::type type;?

I don't think so.

> numeric/numeric.hpp line 3
> /// Defined the mathematical operator s on time series
> ->
> /// Defines the mathematical operator's on time series

Should be "operators". Plural, not possessive. Noted.

> The description of series_stl_iterator in the reference
> has no direct indication of what kind of iterator it is.

Drat, Doxygen stripped out the inheritance from iterator_facade that 
would have shown it to be a forward iterator. But this guy certainly 
needs some better docs.

> Is a time series required to inherit from time_series_base?
> If it is then this should be specified in the concepts somehow.
> If not then the reference for time_series_base should not say
> that it is a base for all time_series.

It's not required to satisfy the concept, but---as you've noticed---some 
of the function templates are currently over-constrained to only work 
with types derived from time_series_base. This needs to be fixed.

> The TimeSeries concepts section lists only dense series as
> models.  Should it list the other time_series's to?

No, that's just an example. I could list every model of the concept in 
the library, but I don't think that would add much.

> The RangeRunStorage concept page has an empty See Also section.

BoostBook bug. Patches welcome. :-)

> This is probably a boostbook issue but there are lots of
> empty namespaces in the reference section.

BoostBook.

> ordered_inserter.hpp line 205
>             range_run_storage::set_at(this->inserter_, 
> std::make_pair(offset, endoff), value);
> should this use run_type instead of std::pair?

Possibly. I seem to recall doing this on purpose, but now I can't 
remember why. :-P I can change it, run the tests, and see what breaks.

> I strongly dislike ordered_inserter.  Its name
> indicates to me that it adds elements to a time_series--not
> at all that it deletes all pre-existing elements.  I would
> expect it to overwrite the old elements only where a new
> element is specified.

That's a valid criticism. Can you suggest a better name?

> This program compiles but fails at runtime.  It should
> fail at compile time.
> 
>     #include <boost/time_series/sparse_series.hpp>
>     #include <boost/time_series/ordered_inserter.hpp>
> 
>     using namespace boost::time_series;
> 
>     typedef sparse_series<double, int, double> series_1;
> 
>     int main() {
>         series_1 s1;
>         make_ordered_inserter(s1)(1.0, 0.0, 1.0).commit();
>     }

Floating-point offsets strike again! I'll fix.

> time_series_facade.hpp line 262
>     #ifdef _MSC_VER
> BOOST_MSVC?
> Other places too.

I don't know the policy here. I'm disabling msvc-specific warnings. Is 
there a case when BOOST_MSVC is defined but _MSC_VER is not? And in that 
case, can I still disable msvc-specific warnings?

> Something somewhere is suppressing "warning C4244: 'initializing' : 
> conversion
> from 'double' to 'int', possible loss of data" and not restoring it but 
> I'm not
> sure whether it's in your library yet.

I'm usually pretty good about pushing and popping the warning levels. If 
I missed one, it's a bug and I'll fix it.

> My IDE (MSVC 8) keeps freezing--Has anyone else noticed this?
> (It could be caused by code in trunk, too)

Me too. <sigh>

> I ran a quick test of sparse_series<quantity<SI::length> > and it worked 
> fine.
> 
> What does discretization mean for floating point offsets?

Same as for integral offsets. It's used to guard against combining 
series with e.g., data sampled daily vs. weekly, and also as a 
multiplicative factor for the integrate() algorithm.

> For sparse/dense series with floating point offsets indexing doesn't 
> make sense.
> Probably it should be made illegal somehow....

It is illegal for dense series already. Sparse series and delta series 
with floating point offsets are currently allowed, but I'm thinking I'll 
disallow those as well so that I can have half-open floating-point ranges.

> I would partly agree with Stjepan about the dichotomy between 
> sparse/dense series
> and piecewise constant series.  As I see it there are three cases
> a) sparse/dense and floating point

On the chopping block.

> b) piecewise constant and floating point
> c) integral
> Because of the limits on integers piecewise constant series
> and point series behave the same way for them.  Multiplication
> makes no sense for point series.  Likewise adjacent_difference,
> integrate, piecewise_sample, piecewise_surface_sample, and index.

I tend to agree, which is why they're not long for this world.

Thanks for your very detailed feedback.

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