search

borglab / GTDynamics

Full kinodynamics constraints for arbitrary robot configurations with factor graphs.
BSD 2-Clause "Simplified" License
39 stars 10 forks source link

Detailed error check #264

Closed dellaert closed 2 years ago

dellaert commented 3 years ago

Added machinery and a csv file to check individual factor error values, checking all 4,467 factor errors in testSpiderWalking by comparing them with stored values in the CSV file tests/testSpiderWalking.csv. This was useful to debug issue #261, but might also allow us to easily explore better initialization strategies (some of which already exist)

dellaert commented 3 years ago

@yetongumich This PR exposes that here as well we have differences between Linux and Mac. If you click on a failing Linux check above you can see that the errors are very different on Linux from the one in the CSV file, on my mac. If you are running this on Linux, could you do a valgrind? Asking you as @varunagrawal and @gchenfc are gunning for ICRA...

dellaert commented 3 years ago

@danbarla also feel free to run valgrind if you're on Linux. Want to get to the bottom of this - might point to (another) weird bug.

dellaert commented 3 years ago

@danbarla I just checked in some regressions on initial values, with expected values from my Mac, and the joint angles and joint velocities do not agree on Linux (used the docker container :-))

/usr/src/GTDynamics/tests/testSpiderWalking.cpp:204: Failure: "expected -1.8499999999999999e-05 but was: 6.0276815148976885e-06" 
/usr/src/GTDynamics/tests/testSpiderWalking.cpp:205: Failure: "expected 1.9759999999999998e-06 but was: -5.1303190182304889e-07" 
/usr/src/GTDynamics/tests/testSpiderWalking.cpp:206: Failure: "expected -1.8499999999999999e-05 but was: 6.0276815148976885e-06" 
/usr/src/GTDynamics/tests/testSpiderWalking.cpp:207: Failure: "expected 1.9759999999999998e-06 but was: -5.1303190182304889e-07" 
/usr/src/GTDynamics/tests/testSpiderWalking.cpp:209: Failure: "expected -3.8999999999999999e-06 but was: 3.7040939584755143e-06" 
/usr/src/GTDynamics/tests/testSpiderWalking.cpp:210: Failure: "expected -5.1100000000000002e-06 but was: 2.597843754075192e-06" 
/usr/src/GTDynamics/tests/testSpiderWalking.cpp:211: Failure: "expected -3.8999999999999999e-06 but was: 3.7040939584755143e-06" 
/usr/src/GTDynamics/tests/testSpiderWalking.cpp:212: Failure: "expected -5.1100000000000002e-06 but was: 2.597843754075192e-06"

That confirms my suspicion, and narrows the problem down to the initialization of joint angles/velocities, as Poses seem to agree.

danbarla commented 3 years ago

@dellaert Please have a look at this link: https://github.com/imneme/pcg-cpp/issues/50

there could be a problem with std::normal_distribution when working cross_platform. The order of the sampled values is changed. actually every pair of values sampled is the same but in the opposite order. This happens even though we use the same seed.

I was able to prove this by adding this line at Sampler.cpp line 43 in GTSAM and recompiling: Normal dist(0.0, sigma); auto res = dist(generator_); result(i) = dist(generator_);

after this the test passes except line 183 (still not sure why)

dellaert commented 3 years ago

@dellaert Please have a look at this link: [https://github.com/[imneme/pcg-cpp/issues/50](https://github.com/imneme/pcg-cpp/issues/50)]

there could be a problem with std::normal_distribution when working cross_platform. The order of the sampled values is changed. actually every pair of values sampled is the same but in the opposite order. This happens even though we use the same seed.

I was able to prove this by adding this line at Sampler.cpp line 43 in GTSAM and recompiling: Normal dist(0.0, sigma); auto res = dist(generator_); result(i) = dist(generator_);

after this the test passes except line 183 (still not sure why)

Woah, good find!!! That is very annoying behavior :-( Can you suggest a way to fix it? Or several options?

dellaert commented 3 years ago

Actually, we don’t have to fix it: randomness in unit tests is frowned upon, anyway, and is actually not needed here. Rather, can we change the test (and possibly the init code) so we can ask for a sequence with no randomness ?

danbarla commented 2 years ago
  1. we could set the gaussian noise to zero. this will just initialize all values to exactly zero.
  2. We could add a boost::optional argument with external initialization to MultiPhaseZeroValuesTrajectory which will come from a csv file. This can help in testing but it will require writing new code and the real code that runs when there is no external initializaion will not be tested.
  3. We can just not test for specific values here. Is this test different from other tests where we initialize with noise and not check for specific values?
dellaert commented 2 years ago

@danbarla I took your zero noise suggestion, implemented it, and merged. I also merged the branch depending on this. Next (after teaching) I will push the branch you and I prototyped in response to the issue I created.