Basis functions #403
Basis functions #403
Conversation
|
Sounds good. 🙂 (PS build failed because Github had an outage around 2:30 am CT last night) |
|
@gchenfc this is the PR I mentioned |
|
Ping @JDFlorezCastillo @luoj1 |
|
FYI I may kill this PR because @dellaert in a previous meeting told me how to go about adding this in. The idea is to basically create a branch in both GTSAM and Double-Expresso and move stuff incrementally so that things are perfectly in tune. I'll also migrate double expresso to github.com so everyone who's tagged can look at the code. It's a lot of clever C++ (written by folks smarter than me) so don't hesitate to ask questions. |
|
Don't kill before we chat |
There was a problem hiding this comment.
Partial review with comments, completed by @gchenfc, @Silverwings-zero, and myself (@JDFlorezCastillo)
|
So most of the comments by @JDFlorezCastillo seems to be in |
Probably that was also because we didn't get through all the files - I think we didn't get to Chebyshev, Chebyshev2, or FitBasis Also, I think most of the comments were superficial in any case |
|
Note to squash and merge. |
gtsam/basis/BasisFactors.h
Outdated
| }; | ||
|
|
||
| /** | ||
| * Factor for scalar BASIS derivative evaluation. |
gtsam/basis/Chebyshev.cpp
Outdated
| * @param t2 New upper limit. | ||
| * @return double | ||
| */ | ||
| double scale(double x, double a, double b, double t1, double t2) { |
gtsam/basis/FitBasis.h
Outdated
| return gfg; | ||
| } | ||
|
|
||
| /// Constructor |
There was a problem hiding this comment.
Add proper doxygen with parameters
gtsam/basis/FitBasis.h
Outdated
| /** | ||
| * Class that does regression via least squares | ||
| * Example usage: | ||
| * auto fit = FitBasis<Chebyshev2>(3, data_points, noise_model); |
| * -------------------------------------------------------------------------- */ | ||
|
|
||
| /** | ||
| * @file ParamaterMatrix.h |
There was a problem hiding this comment.
Rethinking this. Do we really need this? maybe we should just use Eigen::Matrix<double, M, -1> and kill this file.
There was a problem hiding this comment.
I've already explained this in a previous comment. Pasting here verbatim 🙂
Doing
template <int M>
using ParameterMatrix = Eigen::Matrix<double, M, -1>causes the API to become far more verbose and non-intuitive.
E.g., we can no longer do ParameterMatrix<12> X(N) since it gives an error that we tried calling a vector method on a matrix object. We have to instead do
ParameterMatrix<12> X;
X.resize(12, N); // why do we need to specify 12 again here? This is something eigen needs :(In general, the fact that one of the dimensions is dynamic and the other has size greater than 1 makes things non-standard for Eigen since it assumes everything is a Matrix and simple things like X.col don't compile.
I'll add this to the docstring for future reference.
There was a problem hiding this comment.
Our other option is is to just use Matrix<double, -1, -1> but then we lose compile time checks on the matrix size.
There was a problem hiding this comment.
It should be very easy to make a small function that creates the right matrix. Could we replac entire file contents with code below?
template <int M>
using ParameterMatrix = Eigen::Matrix<double, M, -1>;
template <int M>
ParameterMatrix<M> MakeParameters(int N) {
ParameterMatrix<M> P;
P.resize(M, N);
return P;
}
and say
auto P = MakeParameters<12>(100);
There was a problem hiding this comment.
That solves the initialization problem but not the various vector-matrix confusion errors that Eigen throws on compilation.
I did the whole typedef and actually using the matrix, and it didn't compile.
gtsam/basis/basis.i
Outdated
| template <BASIS = {gtsam::FourierBasis, gtsam::Chebyshev1Basis, | ||
| gtsam::Chebyshev2Basis, gtsam::Chebyshev2}> | ||
| class FitBasis { | ||
| FitBasis(size_t N, const std::map<double, double>& sequence, |
There was a problem hiding this comment.
@gchenfc try this out! I hit upon this idea last night.
There was a problem hiding this comment.
@gchenfc gentle reminder.
There was a problem hiding this comment.
SORRY I just saw this, need to tighten up my GitHub notifications to filter out spam, as this was not spam.
Sorry I'm not clear on how to use this without including <pybind/stl.h> ?
|
From Frank: Can move wrapper stuff to different PR. |
|
Wrapper stuff has been fixed. I have to address Frank's comments but I am swamped with Skydio and ICRA so pushing this back a bit. I'll get to it eventually :) |
|
@varunagrawal - you seem to be working on many PR's except this one :-) Let's land it after you address comments, people are waiting for it... |
|
Yeah though those other PRs are related to the state estimator. |
|
@dellaert this is ready for review |
gtsam/basis/BasisFactors.h
Outdated
|
|
||
| /** | ||
| * @brief Factor for enforcing the scalar value of the polynomial BASIS | ||
| * represenation at `x` is the same as the measurement `z` when using a |
|
Congrats @varunagrawal !!!! |
This PR adds support for Basis functions in GTSAM, such as Chebyshev, Chebyshev2 and Fourier.
Additionally it also has code for basis fitting which uses the newly introduced FunctorizedFactor.