Fix visitWith for Pruned Tree

[varunagrawal]

Added a unit test demonstrating a bug in partial assignments of visitWith and added a fix for it.

For example, if we have a pruned tree like below:

 Choice(C)
 0 Choice(B)
 0 0 Leaf 0
 0 1 Choice(A)
 0 1 0 Leaf 2
 0 1 1 Leaf 3
 1 Choice(B)
 1 0 Leaf 4
 1 1 Choice(A)
 1 1 0 Leaf 6
 1 1 1 Leaf 7

if we look at all the assignments that get generated by visitWith, we get

>>: (B, 0)(C, 0)
>>: (A, 0)(B, 1)(C, 0)
>>: (A, 1)(B, 1)(C, 0)
>>: (A, 1)(B, 0)(C, 1)  // This is WRONG!
>>: (A, 0)(B, 1)(C, 1)
>>: (A, 1)(B, 1)(C, 1)

and we should instead get

>>: (B, 0)(C, 0)
>>: (A, 0)(B, 1)(C, 0)
>>: (A, 1)(B, 1)(C, 0)
>>: (B, 0)(C, 1)  // This is CORRECT.
>>: (A, 0)(B, 1)(C, 1)
>>: (A, 1)(B, 1)(C, 1)

[dellaert]

Good find! Please see whether my suggested fix works, as I think that will not touch the complexity.

[dellaert]

Include order convention in gtsam is near -> far. It was already correct except all discrete includes should precede base includes.

[varunagrawal]

Auto-format oopsie

[dellaert]

That seems rather heavy handed. Can we not fix the issue simply by removing the choice after the recursive call? That we way we are not copying binary trees all over the place. Worried that the complexity of this method just became O(n^2) rather than O(n).

[varunagrawal]

Gotcha. The VisitWith struct is a pretty lightweight datastructure so I figured a functional recursive implementation would be alright. Let me update it.

[dellaert]

Cool ! Thanks for finding the bug and fixing it!