Skip to content

Add covariance#3

Merged
efernandez merged 5 commits intoindigo-develfrom
add_covariance
Jun 17, 2015
Merged

Add covariance#3
efernandez merged 5 commits intoindigo-develfrom
add_covariance

Conversation

@efernandez
Copy link
Copy Markdown
Member

Add pose and twist covariance.

Also add tests for the covariance and the twist fixed in #2.

Also roslint has been added and several warnings fixed.

@efernandez
Copy link
Copy Markdown
Member Author

@servos @paulbovbel

I'll squash the commits after your review.

For the positive definite test/check on the covariance matrix I cannot compared against 0 because of numerical issues with Eigen eigensolver for self-adjoint matrices. I've also tried the Cholesky decomposition, and it's only slightly more robuts. With Matlab eig the test passes. As a workaround, which I think it's appropriate in this case, I compared against -EPS. With this, all the tests pass.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This fails (ie vl = 0) if T is an int (not that that is likely). Any reason not to do (vr + vl) / T(2)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

T must be double/float or a dual number like Jet<double/float, N>. It should never be int, but I'm not sure how to avoid that.

I use T(0.5) as it was done before, no difference with dividing by T(2).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Depending how strongly you feel, you can use static_assert to prevent the template being instantiated with inappropriate types, eg:

#include <boost/type_traits.hpp>
static_assert(!boost::is_floating_point<T>::value, "The pose components must be specified as float values.");

(Note that the type_traits module has been brought into the STL for c++11.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good one.

Since C++11 is not allowed upstream, I've used BOOST_ASSERT_MSG instead of static_assert tough.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you're looking for BOOST_STATIC_ASSERT_MSG for compile time

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that's true. Fixed.

It seems that with Boost the message isn't shown, and instead a quite strange compilation error is shown. This doesn't happen with static_assert (C++11) tough. See the two links for comparison purposes here.

I've decided to keep the check (using Boost, since C++11 isn't recommended in ROS as I understand from REP3), since the case when this happens is uncommon and at least the error points to the line with the static assert.

@efernandez
Copy link
Copy Markdown
Member Author

BTW, after make test all tests pass:

100% tests passed, 0 tests failed out of 12

Now there are many more tests, in the sense that for the basic controller test also the covariance is tested.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mikepurvis Should I use a Clearpath Robotics copyright for the new files I've created here? If that's the case, could you provide me a link to an example?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks.

I'll make it 80-column width tough.

@efernandez
Copy link
Copy Markdown
Member Author

@paulbovbel Any comments before I start squashing commits and merging?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mikepurvis Do you know if this is actually needed when cmake_modules is already a build depend, following this?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe so. cmake_modules only gets you the ability to find_package it, but you still need the package itself to be present on the system.

Enrique Fernandez added 2 commits June 16, 2015 23:06
This is needed to compute the covariance properly
Enrique Fernandez added 3 commits June 17, 2015 09:25
1. Moving along x axis covariance grows faster on y axis
2. Moving along y axis covariance grows faster on x axes
Check that the following propagation equations are satisfied:

(x, y) = (x, y) + R(yaw) * (v_x, v_y) * dt
yaw = yaw + v_yaw * dt (normalized)
@efernandez
Copy link
Copy Markdown
Member Author

I've just squashed the commits.
I've also added a comment to make it clear that a covariance matrix is positive semidefinite, but we check for positive definite (up to some EPS precision) because in the tests we know (or want to enforce that) the covariance matrices must be positive definite.

If there are no additional comments, I'll merge this.

efernandez pushed a commit that referenced this pull request Jun 17, 2015
@efernandez efernandez merged commit e97353f into indigo-devel Jun 17, 2015
@efernandez efernandez deleted the add_covariance branch June 17, 2015 17:55
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.

4 participants