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

Fix bug in cubic B spline and enhance class #22

Merged
merged 9 commits into from
Jan 21, 2015

Conversation

florent-lamiraux
Copy link
Member

Add a constructor with a vector of knots,
fix bug in computation of basis functions. The bug has no effect when knots are equally spaced.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.23%) when pulling 704a1df on florent-lamiraux:fix-cubic-b-spline-bug into f912b74 on roboptim:master.

@bchretien
Copy link
Member

👍 Looks good, thanks!

However a test is missing for the new constructor. An example with non-uniform b-splines would probably be the best thing to do, since this was not really tested (and fixed) until now. Copying and adapting the cubic-b-spline tests for the non-uniform case should be a good start.

@florent-lamiraux
Copy link
Member Author

I have added a test that generates basis functions of non-uniform cubic B splines.
The output of the test is understood by gnuplot. The curves look correct.
There are floating point comparisons between the values returned by the basis functions and values stored in a local file.

{
i = Double2SizeType::convert
(std::floor (static_cast<double> (imin) + (t - tmin)
/ (tmax - tmin) * static_cast<double> (imax - imin)));
(std::floor (static_cast<double> (.5*(imin + imax)+.5)));
Copy link
Member

Choose a reason for hiding this comment

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

/home/travis/build/roboptim/roboptim-trajectory/src/cubic-b-spline.cc:196:53: error: conversion to ‘double’ from ‘roboptim::trajectory::Trajectory<3u>::size_type {aka long int}’ may alter its value [-Werror=conversion]

This should probably be:

(std::floor (.5 * static_cast<value_type> (imin + imax) + .5));

@florent-lamiraux
Copy link
Member Author

I fixed the warning. It compiles on my machine.
Let see what travis thinks about it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) when pulling 606e660 on florent-lamiraux:fix-cubic-b-spline-bug into f912b74 on roboptim:master.

@bchretien
Copy link
Member

Looks good. Just a question regarding d6c48e1 (first commit for the gradient test): was it related to a difference in the output for a different version of Eigen? Or a lack of precision in the output?

@florent-lamiraux
Copy link
Member Author

The test failed because of numerical precision. At several places, the last decimal was different. I do not know precisely the reason, eigen version, different os...

bchretien added a commit that referenced this pull request Jan 21, 2015
Fix bug in cubic B spline and enhance class
@bchretien bchretien merged commit f701a7a into roboptim:master Jan 21, 2015
@bchretien
Copy link
Member

👍
I see, thanks for testing and fixing it then, these errors can be difficult to predict.

@florent-lamiraux florent-lamiraux deleted the fix-cubic-b-spline-bug branch January 22, 2015 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants