search

loco-3d / crocoddyl

Crocoddyl is an optimal control library for robot control under contact sequence. Its solver is based on various efficient Differential Dynamic Programming (DDP)-like algorithms
BSD 3-Clause "New" or "Revised" License
859 stars 174 forks source link

Fix unittest, clean up and proper disturbance value for NumDiff - [merged] #457

Closed wxmerkt closed 5 years ago

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 20, 2019, 15:24

Merges fix-unittest -> devel

In this PR I did the follows:

With this PR I fixed all little problems as described here #84. For that, I conducted a stress test for all unit-test (i.e. I run 100 times the test without any failure).

wxmerkt commented 5 years ago

In GitLab by @nmansard on Feb 20, 2019, 15:38

Using 1e-8 for finite diff with eigenpy does not work properly

Please avoid extra complex code like going to 2 lines of code for finally writing 1e-8 (which finally is not working)

wxmerkt commented 5 years ago

In GitLab by @jcarpent on Feb 20, 2019, 15:40

It is because of Numpy, not EigenPy

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 20, 2019, 16:34

@nmansard I'm using $\sqrt{2*eps}\approx 2.1073\cdot 10^{-8}$. Any number lower than $\sqrt{2*eps}$, which is the case of $1\cdot 10^{-8}$ might produce errors in the differentiation. In the same vein, I extensively test this value and it works well.

I'm not aware of Numpy issue, could you explain me? It seems for me that it just practical considerations of NumDiff.

wxmerkt commented 5 years ago

In GitLab by @jcarpent on Feb 20, 2019, 16:41

You always get errors from finite differences. The value you provide is the optimal resolution you can get with floating points.

Numpy is making some roundings that you cannot control, around 1⋅10−8. This why it is very difficult to use Python for finite differences.

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 20, 2019, 16:50

added 9 commits

Compare with previous version

wxmerkt commented 5 years ago

In GitLab by @nmansard on Feb 20, 2019, 18:01

My experience of finite diff with Pinocchio is that rounding errors make it very difficult to have disturbance below 1e-5, which is approx sqrt(1e-9). I would like to believe that it is coming from numpy, but pure numpy functions are much less sensitive: it rather seems to be somewhere between python and pinocchio. Accuracy does not scale as expected with the reduction of the disturbance, and tends to increase when the disturbance is smaller than 1e-8.

Consequence: I don't advice to use disturbance below 1e-6 in crocoddyl, except if you really spend the necessary time to investigate this issue.

wxmerkt commented 5 years ago

In GitLab by @jcarpent on Feb 20, 2019, 18:39

You don't have such difference in C++ with Pinocchio. My experience with Python is that Python is rounding all the computations around 1e-8, adding noise and so on. Mainly when you are providing an increment to the x+h term. You can check this fact.

wxmerkt commented 5 years ago

In GitLab by @nmansard on Feb 21, 2019, 12:58

?

wxmerkt commented 5 years ago

In GitLab by @nmansard on Feb 21, 2019, 13:02

  1. You define this EPS variable. I don't think crocoddyl should externalise an EPS variable. You might define EPS locally, or in the unittest, but not in crocoddyl main context.
  2. You made quite a work on cleaning the numerical test. I hope your did not spend too much time on that. However, I believe you failed the main problem: numerical issues related to python / pinocchio / eigenpy. These problems must be investigated properly before changing the thresholds in the unittest.

If we want to merge this MR: can you assert that the cleaning you did in the numerical threshold are well chosen and well validated? I don't want to spend time on fixing this patch later.

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 22, 2019, 12:04

@nmansard and @jcarpent I didn't encounter any issue, maybe I didn't test it enough. So far I know that it passes always the unit-test, however I think we are just using NumDiff for action models that aren't based on Pinocchio.

I will spend couples of hour to do a benchmark with both cases: pinocchio and non-pinocchio DAMs. I will come back to you when I have these results.

wxmerkt commented 5 years ago

In GitLab by @nmansard on Feb 23, 2019, 12:00

Hi @cmastalli . This work is interesting, however it is not very important for Crocoddyl right now. I am happy if you do it as a break between more important and more complex tasks. Ok?

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 12:42

@nmansard I just spent 20 min to check the raised concerns. I just need here to tackle your other issues. It's important because I'm also solving unit-tests.

I will move soon to more important tasks. Just need to work on this around 30 min.

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 17:27

changed this line in version 3 of the diff

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 17:27

added 26 commits

Compare with previous version

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 17:28

added 1 commit

Compare with previous version

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 17:30

added 1 commit

Compare with previous version

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 17:31

I apologize for this embarrassing mistake. I have no idea what happens here. I reverted the code as before!

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 17:31

resolved all discussions

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 17:41

added 2 commits

Compare with previous version

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 18:35

I tested several disturbance values for the manipulator problem and I didn't find the problem that you raised. These is the average error (norm-2) of 100 computation of the Fx derivative again:

disturbance error
1e-09 2.904689427317484e-07
1e-08 2.8197984659193634e-08
1e-07 3.7610360068076936e-09
1e-06 2.1673772650267067e-08
1e-05 2.167509470764412e-07
0.0001 2.167511860666553e-06
0.001 2.1675726853194042e-05

I did the same trial for the floating in contact system but I found an issue/bug there. Please see #123 for more details.

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 18:40

@nmansard I believe using $h=\sqrt{2*eps}\approx 2e-8$ is a reasonable value. This value is often the best one. And again, I don't detect any rounding problem with Numpy or Pinocchio.

Of course, you might want to have better value for your problem. The user should be responsible of finding it.

I think we can merge this PR.

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 23, 2019, 21:28

added 1 commit

Compare with previous version

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Feb 27, 2019, 09:42

@nmansard you can merge it if there isn't any further comment regarding this.

Please note that it has passed the stress test (i.e. running 100 times all unit-tests without any problem

wxmerkt commented 5 years ago

In GitLab by @nmansard on Feb 27, 2019, 10:30

merged

wxmerkt commented 5 years ago

In GitLab by @nmansard on Feb 27, 2019, 10:30

mentioned in commit 694528737d1e0079e168c37d6bf9f6f8967084dd

wxmerkt commented 5 years ago

In GitLab by @cmastalli on Mar 4, 2019, 09:57

mentioned in merge request !121