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

Feature/6D_Expressions #40

Merged
merged 68 commits into from
Jan 6, 2015
Merged

Feature/6D_Expressions #40

merged 68 commits into from
Jan 6, 2015

Conversation

rdube
Copy link
Contributor

@rdube rdube commented Nov 1, 2014

Hi all,
Here is an attempt to merge a branch on which @gawela, @mikebosse and I have been working for quite some time now. We understand that this is unfortunately not a small PR and it will be troublesome to merge it. These 26 modified files are the result of experimenting with the library, adapting the interface to GTSAM and learning! :-)

Some changes, amongst others :

  • VectorSpaceCurve is now templated on its dimension. This new template functionality resulted in a LinearInterpolationVectorSpaceCurve-inl.h file. Is this the desired implementation for member functions of a class template?
  • All instances of evaluators were removed from VectorSpaceCurve, being replaced by Expressions. Two unit-tests were created, showing a possible usage of Expressions for curves.
  • Implementation of SE3 Slerp curve with a kindr quatTransformation as ValueType. This work included several hours of flushing the curve interface which resulted in modifications all around the code.

*Note : The SE3 curve still make use of evaluators. I believe not much effort should be invested in reviewing this “evaluation” part since it will all change to expressions anyway. In trajectory maintainer, we had working unit tests for “big” 6DOF data sets but they will also need to adapted accordingly.

@sanderson77 sorry if this breaks your stuff. This was to be expected with the implementation of Expressions. Good news is, these Expressions are very friendly and make the evaluation interface much cleaner.

I will try to add some comments to help with the review process.

We look forward to hear your comments. Thanks!

furgalep and others added 22 commits October 3, 2014 18:41
@ethzasl-jenkins
Copy link

Can one of the admins verify this patch?

@furgalep
Copy link
Contributor

furgalep commented Nov 1, 2014

test this please

@rdube
Copy link
Contributor Author

rdube commented Nov 1, 2014

Somehow complaining about the traits function :

/var/lib/jenkins/jobs/curves/workspace/label/ubuntu/src/curves/test/test_Expressions.cpp:23:23: error: ‘equals’ is not a template function
double tol) {

Will check into this eventually.

@furgalep
Copy link
Contributor

furgalep commented Nov 1, 2014

@rdube Can you fix the failing build?

@HannesSommer
Copy link

as a hint: apparently the template wasn't declared in this namespace before its specialization in test_Expressions.cpp:23. Either you forgot an include or you are in the wrong namespace there.

@rdube
Copy link
Contributor Author

rdube commented Nov 3, 2014

test this please

@rdube
Copy link
Contributor Author

rdube commented Dec 1, 2014

Following the discussion of #42 Jenkins has been told to use rev 61666f22d618010bc1a63298ef856741db62f1d3 of gtsam.

@mikebosse do you agree with developing our libraries using this gtsam revision for a while? Should we consider merging this PR?

@mikebosse
Copy link
Contributor

A lot of good updates are going into the expressions for gtsam. I think it
would be good to stick with the latest for a while longer in stead of
freezing with an intermediate version. That way we can incrementally keep
up with the best from gtsam rather than having to make a major refactor in
the future.

On Monday, December 1, 2014, rdube notifications@github.com wrote:

Following the discussion of #42
#42 Jenkins has been told to
use rev 61666f22d618010bc1a63298ef856741db62f1d3 of gtsam.

@mikebosse https://github.com/mikebosse do you agree with developing
our libraries using this gtsam revision for a while? Should we consider
merging this PR?


Reply to this email directly or view it on GitHub
#40 (comment).

@rdube
Copy link
Contributor Author

rdube commented Dec 1, 2014

OK thanks Mike. Well understood! So I pulled the latest revision of GTSAM both locally and on Jenkins. Everything builds and the tests are running. We are up to date!

@sanderson77
Copy link
Contributor

I agree that keeping up to date with incremental changes over a major refactor is preferable, but we need to make sure we clearly communicate what revision we are on (and Jenkins is using). Getting out of sync can be messy, and gtsam is moving pretty quick!

Is there somewhere the gtsam revision number curves is building on can be posted? or checked?

e.g. @rdube is the "latest" gtsam you are describing, rev 133dbc4dede0defae8e736fea5c4e581d13c6a06?

@mikebosse
Copy link
Contributor

Ok, then every time we update curves with GTSAM, we will send you an email
with the revision number.

On Mon, Dec 1, 2014 at 8:37 PM, Sean Anderson notifications@github.com
wrote:

I agree that keeping up to date with incremental changes over a major
refactor is preferable, but we need to make sure we clearly communicate
what revision we are on (and Jenkins is using). Getting out of sync can be
messy, and gtsam is moving pretty quick!

Is there somewhere the gtsam revision number curves is building on can be
posted? or checked?

e.g. @rdube https://github.com/rdube is the "latest" gtsam you are
describing, rev 133dbc4dede0defae8e736fea5c4e581d13c6a06?


Reply to this email directly or view it on GitHub
#40 (comment).

@rdube
Copy link
Contributor Author

rdube commented Dec 2, 2014

@sanderson77 Yes the latest was 133dbc4dede0defae8e736fea5c4e581d13c6a06. I am now trying to build d7bf997197db037fc6244f568cd685100c87b731 but I get a linking error concerning wrap. The problem is in GTSAM itself - should not be related to curves.

@mikebosse
Copy link
Contributor

I was able to get the build d7bf997197db037fc6244f568cd685100c87b731 to compile on my mac, no problems. But I had to first remove my build directory and start from scratch...

@rdube
Copy link
Contributor Author

rdube commented Dec 2, 2014

Ok good. We are up to date with d7bf997197db037fc6244f568cd685100c87b731 (on jenkins as well).

furgalep added a commit that referenced this pull request Jan 6, 2015
@furgalep furgalep merged commit 516d43a into master Jan 6, 2015
@rdube rdube deleted the feature/6D-Expressions branch January 16, 2015 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants