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

Topic/integration #66

Merged
merged 21 commits into from
Jul 13, 2022
Merged

Topic/integration #66

merged 21 commits into from
Jul 13, 2022

Conversation

aescande
Copy link
Contributor

@aescande aescande commented Jul 9, 2019

This PR reworks the numerical integration for the non-trivial joints (planar, spherical and free):

  • fix a sign error in the Magnus expansion and conditionally extend the expansion to 5 terms, depending on a user-defined precision,
  • properly take into account that translation velocities are expressed in the successor frames (FS) by integrating the motion of this frame instead of considering it constant over the integration step,
  • the error done during the integration is estimated and used to subdivide the integration step if necessary. Consequently, integration can be carried out on large intervals (tested up to 1 second),
  • new tests are added, old ones are reworked
  • detailed mathematical notes are added in the doc folder. Not yet linked to the doxygen.
  • change name from eulerIntegration to integration or numericalIntegration to better reflect what's happening in the functions. Keep old names around but marked as deprecated.

This PR is not polished yet, but I'd like to have some general feedback first/while polishing.
First of all the tests. I'm currently performing 4 types:

  • comparing integration at constant velocity with analytical formula (for constant velocity it is possible to derive these formula, which is not the case for constant acceleration)
  • comparing the integration at constant acceleration with step dt to N integrations at constant velocity with step dt/N, changing the velocity between those integrations. N is a large number (10000 right now)
  • checking the self-consistency of the integration: one integration with step dt should yield the same result as N integrations with step dt/N.
  • checking the consistency of the integration with the jacobian matrix in the following way: I take a point P on the body after the joint. Taking a constant velocity v for the joint, we should get the same result by
    (i) integrating the joint position and computing the new position of P
    (ii) integrating Jv, where J is the Jacobian matrix of the point.

The last test is an attempt to ensure that the integration is coherent with the library as a whole, because it could be possible to misunderstand the conventions adopted in RBDyn and write a wrong integration that could however pass the 3 first tests. The idea of using the Jacobian matrix is to get quantities in the world frame, where there is no specific convention, from whatever the original spaces are for the joint variables and velocities/accelerations. If anyone has a better idea, I'd be happy to hear it.
Something related is done in EulerTestV2 in AlgoTest, but quantites are brought back in some variable space, which makes the test dependent on conventions, and makes it actually wrong for planar and free joints (although because of the very low accuracy required, the planar test is still passing - but I had to disable the one for free joint). I believe this test should be removed completely.

Another point is that the estimation of the error is adding some overhead. I didn't measure it for now, as it is up to a few dozens of scalar additions/multiplications, but it could be equivalent to the integration computation in itself. Does it seems ok? Or do I add a flag/versions without any precision check?

The PR solves #35, without addressing #65 (I could not get an integration coherent with the Jacobian matrix when changing the convention for the planar joint, and I don't get where I'm making a mistake).

@aescande
Copy link
Contributor Author

@gergondet : there does not seems to be a mechanism for deprecation in RBDyn yet. Do you have any guidelines on how to proceed?
Here are a few options I see:

@aescande
Copy link
Contributor Author

Baring feedbacks, I'm done with the code so far.
Code review can start :)

Copy link

@vsamy vsamy left a comment

Choose a reason for hiding this comment

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

The code seems to be clean. There is just some few changes.
Although, you might need to transform your if (...) ...; in

if (...)
{
  ...
}

src/NumericalIntegration.cpp Outdated Show resolved Hide resolved
src/RBDyn/NumericalIntegration.h Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/NumericalIntegration.cpp Outdated Show resolved Hide resolved
src/RBDyn/NumericalIntegration.h Outdated Show resolved Hide resolved
@gergondet gergondet force-pushed the topic/integration branch 2 times, most recently from e17181b to cb6da09 Compare October 31, 2019 09:07
Copy link
Member

@gergondet gergondet left a comment

Choose a reason for hiding this comment

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

Sorry for the long time it took me to get to it. I don't have much issues with the PR, just small readability/efficiency concerns. I've rebased on the current master so you should make sure that you nuke your local branch before pushing back to it

I might be possible to integrate your latex note in the doxygen. I'll look into it

src/IS.cpp Outdated Show resolved Hide resolved
src/NumericalIntegration.cpp Outdated Show resolved Hide resolved
src/NumericalIntegration.cpp Outdated Show resolved Hide resolved
src/NumericalIntegration.cpp Outdated Show resolved Hide resolved
src/NumericalIntegration.cpp Outdated Show resolved Hide resolved
src/NumericalIntegration.cpp Outdated Show resolved Hide resolved
src/RBDyn/EulerIntegration.h Outdated Show resolved Hide resolved
src/RBDyn/EulerIntegration.h Outdated Show resolved Hide resolved
src/RBDyn/EulerIntegration.h Outdated Show resolved Hide resolved
src/RBDyn/NumericalIntegration.h Outdated Show resolved Hide resolved
@gergondet gergondet merged commit 071d681 into jrl-umi3218:master Jul 13, 2022
gergondet added a commit that referenced this pull request Jul 25, 2022
- Always generate URIs in the URDF (#90)
- Honor GNU install dirs (#91)
- Fix tests on aarch64 (#92)
- Improve numerical integration for free-flyer and spherical joints (#66)
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.

3 participants