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