ethz-asl / curves

A library of curves for estimation.
BSD 3-Clause "New" or "Revised" License
74 stars 28 forks source link

Coefficient Manager Generalization #44

Closed sanderson77 closed 9 years ago

sanderson77 commented 9 years ago

First attempt at generalizing the coefficient manager based on the current architecture.

A little renaming: 1) CoefficientManagerBase now serves to provide basic functionality, separate from any type of curve 2) Support2CoefficientManager has the exact functionality as the old HermiteCoefficientManager (a curve with local support 2) 3) SdeGaussianProcessCoefficientManager serves as an example to how I intend to extend the coefficient managers

Changes: 1) Changed maps to use shared pointers so that the resources being held can be extended (coefficient managers can now store derivatives of KeyTimeCoefficient). Is there any concern about shared pointers being slow? 2) Added a copy constructor and assignment operator (this was missing before) 3) SdeGaussianProcessCoefficientManager acts as an example of using a derived KeyTimeCoefficient and demos why some of the functions are now virtual.

Side note: @furgalep @mikebosse @rdube @gawela I'd appreciate comments or concerns about these changes. As far as merging goes, I was curious what the timeline is like on merging the 6D-Expressions feature? That pull request is fairly old and large, and this feature will only complicate it further.

In the mean time, I intend to branch off of this feature to continue writing the GP code.

ethzasl-jenkins commented 9 years ago

Can one of the admins verify this patch?

furgalep commented 9 years ago

I need a day or two to find time to review.

furgalep commented 9 years ago

Okay, I didn't go through this with a fine-toothed comb but I get the idea. Here are some comments:

  1. Thanks for keeping the PR small! This is great!
  2. Support2CoefficientManager is a good name.
  3. If I get the idea, the main point is that you would like to keep extra stuff that won't be estimated along with the keys coefficients and times. If this is the case, the most straightforward design would be to template the coefficient manager on the value type. Then you would have something like: KeyTimeValue instead of KeyCoefficientTime. Would that solve all of your problems? I like that design better. Then all this stuff with clone() and everything goes away. Right?
sanderson77 commented 9 years ago

Yes you have the right idea. I need to store some extra vectors and matrices associated with the prior, evaluated at the same times as the coefficients.

I thought about templating at first, but decided against it for a few reasons. First of all, I am a little unclear on exactly how you envision the templating to occur, is your suggestion something like:

template<typename ContainerType>
class CoefficientManagerBase {
...
  boost::unordered_map<Key, ContainerType > coefficients_;
}

or

template<typename ValueType>
struct KeyTimeValue {
  Key key;
  Time time;
  Coefficient coeff;
  ValueType value;
}

template<typename ValueType>
class CoefficientManagerBase {
...
  boost::unordered_map<Key, KeyTimeValue<ValueType> > coefficients_;
}

1) The manager should always store Key, Time and Coefficients, and in some cases, additional values. In the first template example, the existence of Key, Time and Coeff is not really enforced (except maybe by the compiler). In the second example, an option of "no extra values" is not really available... 2) At the moment, the following function is part of the Support2CoefficientManager, exposed through the Curve classes, and then used in many many places..

bool getCoefficientsAt(Time time, KeyCoefficientTime** outCoefficient0, KeyCoefficientTime** outCoefficient1) const;

I chose polymorphism because regardless of whether additional values are being stored, outside classes only need to know the form of the base container type, KeyCoefficientTime.

furgalep commented 9 years ago

Me and @mikebosse are going to refresh our memories and try to look at this by the end of the week.

sanderson77 commented 9 years ago

No problem. I have been working on the SE(3) math for the sparse GP.

Looking back on this, templating a "container type" might be the right way to go, with the default being KeyCoefficientTime.