[boost] [time_series] Review

From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2007-08-07 20:00:12

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

"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?

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

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

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

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

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> {};

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

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;
    }

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

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.

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?

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;?

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

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

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.

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

The RangeRunStorage concept page has an empty See Also section.

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

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?

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.

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();
    }

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

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.

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

I ran a quick test of sparse_series<quantity<SI::length> > and it worked 
fine.

What does discretization mean for floating point offsets?

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

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

In Christ,
Steven Watanabe