-
Notifications
You must be signed in to change notification settings - Fork 767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basis functions #403
Basis functions #403
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Though I have reservations about basis folder, and some other comments - but procrastinated a bit too much and now need full attention for RSS and ECCV - propose to defer to 4.1
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do respond to all JD's comments in a meaningful way. I thought they were very germane and provided a set of fresh eyes. I helped below, and I think we have to resolve two main issues before next review: (a) unsupported, (b) Weights.
Note to squash and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of comments in Basis.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting there, but before adding to GTSAM we really need better docs.
Please reply to every comment (with Done or thumbs up for most) which makes it really easy to re-review.
gtsam/basis/BasisFactors.h
Outdated
}; | ||
|
||
/** | ||
* Factor for scalar BASIS derivative evaluation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain.
gtsam/basis/Chebyshev.cpp
Outdated
* @param t2 New upper limit. | ||
* @return double | ||
*/ | ||
double scale(double x, double a, double b, double t1, double t2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
gtsam/basis/FitBasis.h
Outdated
return gfg; | ||
} | ||
|
||
/// Constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 should be last argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* -------------------------------------------------------------------------- */ | ||
|
||
/** | ||
* @file ParamaterMatrix.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gchenfc try this out! I hit upon this idea last night.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gchenfc gentle reminder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting really close. A few more requested comments.
@dellaert this is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome !!!!! One little spelling nit.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
representation
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.