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

[Estimation] Add DiscreteKalmanFilterHelper class #559

Merged
merged 1 commit into from
Aug 21, 2019

Conversation

prashanthr05
Copy link
Contributor

This PR adds a DiscreteKalmanFilterHelper class which implements a discrete, linear time-invariant Kalman Filter.
The steps to construct and implement the Kalman filter is described in the documentation and also in a KalmanFilterUnitTest implementation.

The unit test, however, tests only the implementation details of the filter and not the results and performance of the filter itself.

@prashanthr05
Copy link
Contributor Author

prashanthr05 commented Aug 20, 2019

@fjandrad @traversaro @MiladShafiee it would be great if you could review this PR. Thanks in advance.

@prashanthr05
Copy link
Contributor Author

The Travis with Valgrind tests seems to fail with the std random number generator. However, when I run the Valgrind tests on my system, it passes without any errors.

@@ -42,6 +42,7 @@ KinDynComputations finally reached feature parity with respect to DynamicsComput
* Added a new version of `changeFixedFrame` in `SimpleLeggedOdometry`class. This can be used to set a desired homogeneous transformation for the fixed frame
* Added `bindings` for `AttitudeMahonyFilter`, `AttitudeQuaternionEKF`, `DiscreteExtendedKalmanFilterHelper` (https://github.com/robotology/idyntree/pull/522)
* Added basic tests for the Attitude Estimator classes (https://github.com/robotology/idyntree/pull/522)
* Added `DiscreteKalmanFilterHelper` class for an implementation of a discrete, linear time-invariant Kalman Filter
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Added `DiscreteKalmanFilterHelper` class for an implementation of a discrete, linear time-invariant Kalman Filter
* Added `DiscreteKalmanFilterHelper` class for an implementation of a discrete, linear time-invariant Kalman Filter (https://github.com/robotology/idyntree/pull/559)

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Minor comments. It would be great if we could actually test the estimated value after 20 iterations, but that I do not think it is strictly required.
Removing the use of std::random_device I think should solve all problems with Valgrind.

const double stddev = 1.0;
std::mt19937 generator(std::random_device{}());
auto dist = std::bind(std::normal_distribution<double>{mean, stddev},
std::mt19937(std::random_device{}()));
Copy link
Member

Choose a reason for hiding this comment

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

I strongly suggest not to use actually random number as seed for the test, as the day the test will actually fail, you will never be able to reproduce the failing test. Just use an arbitrary deterministic seed for the RNG, and this will make the test deterministic.

Copy link
Member

Choose a reason for hiding this comment

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

Avoiding non-determinism in tests is also useful if we ever want to do fuzzing, see https://llvm.org/docs/LibFuzzer.html#fuzz-target .

Copy link
Contributor Author

@prashanthr05 prashanthr05 Aug 21, 2019

Choose a reason for hiding this comment

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

Removed std::random_device and and used stdlib's srand and rand instead. Fixed in the recent commit 2a25958.

@traversaro
Copy link
Member

traversaro commented Aug 21, 2019

However, when I run the Valgrind tests on my system, it passes without any errors.

I think the fact is that in the CI system the Valgrind used is the one of Xenial, and you are using the one in Bionic (3.11 vs 3.13, see https://repology.org/project/valgrind/versions). However, I think avoiding to use std::random_device should solve any problem.

@prashanthr05
Copy link
Contributor Author

It would be great if we could actually test the estimated value after 20 iterations, but that I do not think it is strictly required.

How do you suggest we test it?

@traversaro traversaro merged commit 41b91b2 into robotology:devel Aug 21, 2019
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.

2 participants