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

817 stars 167 forks source link

test_diff_actions fails for clang as part of make test #964

Closed wxmerkt closed 2 years ago

wxmerkt commented 3 years ago

Noticed when testing different clean Docker CI configs based on Ubuntu 20.04; and repeatable across dozens of runs:

all unit tests pass with gcc
with clang, test_diff_actions repeatedly fails as part of make test (The following tests FAILED: 15 - test_diff_actions (Child aborted). When running the executable independently (./build/unittest/test_diff_actions), it passes and works fine.

cmastalli commented 3 years ago

I have observed this issue locally, but I don't know what it's going on.

Could this be related with the linked boost unittest library?

I appreciate any feedback in this topic.

wxmerkt commented 2 years ago

I found the source for part of this issue (test_diff_actions, haven't started debugging test_actions yet) and it's in https://github.com/loco-3d/crocoddyl/blob/43502019fe5f744d0e20588815625467c69aab68/include/crocoddyl/multibody/contacts/multiple-contacts.hxx#L63-L84

In particular in how the active_ and inactive_ elements are updated. They are currently defined as std::vector<std::string>. I'd like to propose to change them to either std::set<std::string> if we care about order or std::unordered_set<std::string> to simplify the code and reduce the possibility of bugs in the logic. What do you think of this API change? The runtime complexity for inserting into/removing from a set is slower, but at the size of the number of contacts it shouldn't really matter

cmastalli commented 2 years ago

Hi @wxmerkt

Are you sure that this code creates issues in test_diff_actions? If so, could you report it here?

On the other hand, we care about the order. It is better to go for std::set. I am happy if you go for it, but (be aware) that this might be tricky to bind. (And we need to bind these objects)

wxmerkt commented 2 years ago

1025 deactives the `test_diff_action` unittest when compiled with clang. It can pass locally but still fails on the buildserver. Since we are increasing coverage on the buildserver and it's tested with g++, let's keep it deactivated for now and this issue open until we can find a proper fix.

loco-3d / crocoddyl

test_diff_actions fails for clang as part of make test #964

1025 deactives the test_diff_action unittest when compiled with clang. It can pass locally but still fails on the buildserver. Since we are increasing coverage on the buildserver and it's tested with g++, let's keep it deactivated for now and this issue open until we can find a proper fix.

1025 deactives the `test_diff_action` unittest when compiled with clang. It can pass locally but still fails on the buildserver. Since we are increasing coverage on the buildserver and it's tested with g++, let's keep it deactivated for now and this issue open until we can find a proper fix.