Feature/6D_Expressions - Githubissues

rdube commented 9 years ago

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!

ethzasl-jenkins commented 9 years ago

Can one of the admins verify this patch?

furgalep commented 9 years ago

test this please

rdube commented 9 years ago

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 commented 9 years ago

@rdube Can you fix the failing build?

HannesSommer commented 9 years ago

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 commented 9 years ago

test this please

rdube commented 9 years ago

Thanks @HannesSommer for fixing the failing build!

rdube commented 9 years ago

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 commented 9 years ago

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 https://github.com/ethz-asl/curves/issues/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 https://github.com/ethz-asl/curves/pull/40#issuecomment-65082265.

rdube commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 https://github.com/ethz-asl/curves/pull/40#issuecomment-65121527.

rdube commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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

ethz-asl / curves

Feature/6D_Expressions #40