Test cases for Fallbacks container #280

j-kuehn · 2021-07-12T07:20:30Z

I added three test cases for the Fallbacks container:

ConnectStageInsideFallbacks shows that adding a Connect stage to the Fallbacks container seems to be broken, as no solution can be computed for the task currently.
ComputeFirstSuccessfulStageOnly suggests that solutions inside the Fallbacks container should not be computed after one solution has been found. I'm not sure how the container was originally intended to work, so this might not be a bug.
ActiveChildReset reveals the failure of the container to reset the active_child_ for every spawned solution from a generator. In this test case, the first ForwardMockup does not become the active_child_ for the third spawned solution from the GeneratorMockup.

v4hn

@j-kuehn Thank you for the discussed tests! I would appreciate if you could provide a fix for ActiveChildReset as well. 🐈

@rhaschke I would appreciate your opinion on the second issue. I strictly prefer the behavior in the test (don't run alternatives if a solution succeeds) but if you prefer we can add an option to toggle behaviors instead.

v4hn · 2021-07-12T21:16:53Z

core/test/test_container.cpp

+	t.add(std::move(fallbacks));
+
+	EXPECT_TRUE(t.plan());
+	EXPECT_EQ(t.numSolutions(), 4u);


Depending on the preferred semantics (whether fallback alternatives should be computed at all), the test is still not correct: If no alternatives should be considered you would have to check for two solutions where both should come from the first ForwardMockup.

Let's wait for Robert's response before modifying this.

I don't yet fully understand the purpose of this test. I would expect two solutions here, contributed by the first child. However, this was not yet tested here. I pushed a corresponding fixup.

There should be another test case, where the external generator is failing first before succeeding.
Currently, I would expect this test to fail (with a solution of the wrong, second child).

The test suggested that after computing solutions for the second spawned solution from the generator, the active_child_ would not be reset to the first child for computing the third solution. Before your two commits, this led to the first child being ignored for the third spawned generator solution.

As @v4hn pointed out, this test was not consistent with the proposed functionality in ComputeFirstSuccessfulStageOnly, but rather tried to test the status quo. But even after your fix for the ActiveChildReset test, I would have expected the test to fail, because as I understand it, the active_child_ is pointing to the second child after the second spawned generator solution. So for the third solution, I expected the container to use the second child for computation. However, the test passes, so it is actually using the first child. I'm not sure how your commit fixed this.

I just tried adding another test as you suggested, where the external generator is failing first before succeeding:

std::make_unique<GeneratorMockup>(PredefinedCosts{ { INF, 0.0 } }));

However, this yields 0 solutions and I'm not sure why.

rhaschke

Thanks for providing these tests. See detailed comments below.
I suggest to disable/skip ConnectStageInsideFallbacks for now as it won't easily fix.
@v4hn: Do you already have concrete plans, how to implement a topological processing order?

rhaschke · 2021-07-13T20:13:06Z

core/test/test_container.cpp

+	t.add(std::move(fallbacks));
+
+	EXPECT_TRUE(t.plan());
+	EXPECT_EQ(t.numSolutions(), 4u);


I don't yet fully understand the purpose of this test. I would expect two solutions here, contributed by the first child. However, this was not yet tested here. I pushed a corresponding fixup.

rhaschke · 2021-07-13T20:33:58Z

core/test/test_container.cpp

 	EXPECT_EQ(t.solutions().size(), 0u);
 }
+
+TEST(Fallback, ConnectStageInsideFallbacks) {


This fails for the following reason:

The Generator spawns a solution, which is correctly forwarded to the start pull interface of Connect.

However, because there was no state pushed into the end pull interface of Connect, canCompute() returns false
and the Fallbacks container forwards to the next child (which doesn't exist).

This can only be solved, if we drop the simple linear execution order and switch to topological execution order.

Sounds reasonable to disable this test for now.

rhaschke · 2021-07-23T06:49:18Z

Ping @v4hn, @j-kuehn. Any thoughts on my feedback?

This should work, but will require more changes.

v4hn

Thanks for the feedback. I wanted to leave it to @j-kuehn to implement a fix for at least the simple test, but it looks like we disagree on what exactly should happen? Please give your reasoning to my comment below.

v4hn · 2021-07-26T12:40:02Z

core/src/container.cpp


+		if (!Stage::solutions().empty())
+			return false;  // we are done as soon as a child has found solutions
+


This patch does not make any sense to me.

If the generator in ComputeFirstSuccessfulStageOnly would spawn multiple solutions, I would still expect a solution from the first child for each of the inputs! Otherwise you prune too many computation branches and loose solutions.

v4hn · 2021-07-26T12:59:59Z

core/test/test_container.cpp

 	EXPECT_TRUE(t.plan());
-	EXPECT_EQ(t.numSolutions(), 4u);
+	EXPECT_EQ(t.numSolutions(), 2u);
+	EXPECT_EQ(first->solutions().size(), 2u);


It seems we agree on how this test should behave. That's good. :)

However, this only succeeds because of https://github.com/ros-planning/moveit_task_constructor/pull/280/files#diff-5b0930974b77eab73f48b74214a60ac0562cd2a110285279bb5ad1094b40a842R822 now (Robert fixes the active child as soon as it produced any solution, even if it can't compute in between). As I do not agree with this patch we are not done here yet.

codecov · 2021-07-26T13:07:42Z

Codecov Report

Merging #280 (414964a) into master (74d33c4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   51.61%   51.64%   +0.04%     
==========================================
  Files         101      101              
  Lines        6587     6588       +1     
==========================================
+ Hits         3399     3402       +3     
+ Misses       3188     3186       -2

Impacted Files	Coverage Δ
core/src/container.cpp	`70.82% <100.00%> (+0.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74d33c4...414964a. Read the comment docs.

v4hn · 2021-09-03T10:51:33Z

superseded by #287

j-kuehn and others added 2 commits July 8, 2021 08:10

add unit tests for Fallbacks container

f2b12f1

fix ActiveChildReset test / make it fail

f47f9c2

j-kuehn changed the title ~~Fallback tests~~ Test cases for Fallbacks container Jul 12, 2021

v4hn reviewed Jul 12, 2021

View reviewed changes

rhaschke added 2 commits July 13, 2021 21:52

Fix test Fallbacks.ComputeFirstSuccessfulStageOnly

b98ee7f

Fix test Fallback.ActiveChildReset

514021c

rhaschke approved these changes Jul 13, 2021

View reviewed changes

disable ConnectStageInsideFallbacks

414964a

This should work, but will require more changes.

v4hn reviewed Jul 26, 2021

View reviewed changes

rhaschke mentioned this pull request Aug 12, 2021

"Fallback" container segfaults the program where "Alternates" container suceeds #283

Closed

v4hn mentioned this pull request Sep 1, 2021

Fallbacks per solution #287

Closed

v4hn closed this Sep 3, 2021

rhaschke mentioned this pull request Nov 25, 2021

Fallbacks reworked: Factorize FallbacksPrivate into 3 separate classes #311

Merged

5 tasks


		if (!Stage::solutions().empty())
		return false; // we are done as soon as a child has found solutions

Test cases for Fallbacks container #280

Test cases for Fallbacks container #280

Uh oh!

Conversation

j-kuehn commented Jul 12, 2021

Uh oh!

v4hn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhaschke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Jul 23, 2021

Uh oh!

v4hn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

v4hn commented Sep 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 26, 2021 •

edited

Loading