Skip to content
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

Merged
merged 42 commits into from
Aug 27, 2021
Merged

Basis functions #403

merged 42 commits into from
Aug 27, 2021

Conversation

varunagrawal
Copy link
Collaborator

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.

@varunagrawal varunagrawal added the feature New proposed feature label Jul 13, 2020
@varunagrawal varunagrawal self-assigned this Jul 13, 2020
@dellaert dellaert added this to the GTSAM 4.1 milestone Jul 13, 2020
Copy link
Member

@dellaert dellaert left a 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

@varunagrawal
Copy link
Collaborator Author

Sounds good. 🙂

(PS build failed because Github had an outage around 2:30 am CT last night)

@varunagrawal varunagrawal removed this from the GTSAM 4.1 milestone Jul 13, 2020
@dellaert dellaert added this to the GTSAM 4.1.1 milestone Jul 14, 2020
@dellaert
Copy link
Member

@gchenfc this is the PR I mentioned

@dellaert dellaert requested a review from gchenfc April 8, 2021 13:32
@dellaert
Copy link
Member

dellaert commented Apr 8, 2021

Ping @JDFlorezCastillo @luoj1

@varunagrawal
Copy link
Collaborator Author

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.

@dellaert
Copy link
Member

dellaert commented Apr 8, 2021

Don't kill before we chat

Copy link

@JD-Florez JD-Florez left a 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)

gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/tests/testFourier.cpp Outdated Show resolved Hide resolved
gtsam/basis/tests/testFourier.cpp Outdated Show resolved Hide resolved
gtsam/basis/tests/testFourier.cpp Outdated Show resolved Hide resolved
gtsam/basis/tests/testFourier.cpp Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator Author

So most of the comments by @JDFlorezCastillo seems to be in Fourier.h which was something @dellaert mainly worked on. I'll take the feedback into account but I may need help understanding some aspects myself.

@gchenfc
Copy link
Member

gchenfc commented May 5, 2021

So most of the comments by @JDFlorezCastillo seems to be in Fourier.h which was something @dellaert mainly worked on. I'll take the feedback into account but I may need help understanding some aspects myself.

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

Copy link
Member

@dellaert dellaert left a 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.

gtsam/basis/tests/testFourier.cpp Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
cmake/HandleEigen.cmake Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator Author

Note to squash and merge.

Copy link
Member

@dellaert dellaert left a 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

gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Outdated Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/Basis.h Show resolved Hide resolved
gtsam/basis/BasisFactors.h Show resolved Hide resolved
gtsam/basis/BasisFactors.h Outdated Show resolved Hide resolved
Copy link
Member

@dellaert dellaert left a 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 Show resolved Hide resolved
gtsam/basis/BasisFactors.h Show resolved Hide resolved
gtsam/basis/BasisFactors.h Show resolved Hide resolved
};

/**
* Factor for scalar BASIS derivative evaluation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain.

* @param t2 New upper limit.
* @return double
*/
double scale(double x, double a, double b, double t1, double t2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return gfg;
}

/// Constructor
Copy link
Member

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

/**
* Class that does regression via least squares
* Example usage:
* auto fit = FitBasis<Chebyshev2>(3, data_points, noise_model);
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

gtsam/basis/Fourier.h Show resolved Hide resolved
* -------------------------------------------------------------------------- */

/**
* @file ParamaterMatrix.h
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Member

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

Copy link
Collaborator Author

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 Show resolved Hide resolved
template <BASIS = {gtsam::FourierBasis, gtsam::Chebyshev1Basis,
gtsam::Chebyshev2Basis, gtsam::Chebyshev2}>
class FitBasis {
FitBasis(size_t N, const std::map<double, double>& sequence,
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gchenfc gentle reminder.

Copy link
Member

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

@gchenfc
Copy link
Member

gchenfc commented Aug 12, 2021

From Frank:
What's the status on this? Is something still blocking this?

Can move wrapper stuff to different PR.

@varunagrawal
Copy link
Collaborator Author

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 :)

@dellaert
Copy link
Member

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

@varunagrawal
Copy link
Collaborator Author

Yeah though those other PRs are related to the state estimator.

Copy link
Member

@dellaert dellaert left a 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.

gtsam/basis/BasisFactors.h Show resolved Hide resolved
gtsam/basis/BasisFactors.h Show resolved Hide resolved
gtsam/basis/BasisFactors.h Show resolved Hide resolved
gtsam/basis/Chebyshev2.h Show resolved Hide resolved
gtsam/basis/FitBasis.h Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator Author

@dellaert this is ready for review

Copy link
Member

@dellaert dellaert left a 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.


/**
* @brief Factor for enforcing the scalar value of the polynomial BASIS
* represenation at `x` is the same as the measurement `z` when using a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

representation

@varunagrawal varunagrawal merged commit ff7ddf4 into develop Aug 27, 2021
@varunagrawal varunagrawal deleted the feature/basis branch August 27, 2021 12:10
@dellaert
Copy link
Member

Congrats @varunagrawal !!!!

This was referenced Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New proposed feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants