GTSAM_DT_MERGING Flag

[varunagrawal]

This PR is a discussion venue for issue #1496.

I have added a unit test which demonstrates an issue with the number of assignments being set for the decision tree.
This seems to be because the decision tree rearranges the keys to be highest to lowest but doesn't account for the implicit pruning performed. If I disable the implicit pruning in the decision tree, this issue doesn't appear.

This PR adds a new CMake flag GTSAM_DT_MERGING which enables/disables DecisionTree merging of branches when the leaves have the same value. This flag is generally useful for debugging and maybe useful for other things.

Accordingly I have updated the tests and the config files.

I have also removed the nrAssignments_ variable in DecisionTree due to it needing significant refactor and the issue it was originally intended for (hybrid pruning) being resolved via alternative means.

[dellaert]

Please give examples as to what this is, I think "number of assignments" needs to be explained.

[varunagrawal]

TODO(Frank): Add comments to mention that this is a convenience function and not used for any major operation.

[dellaert]

@varunagrawal I think we discussed this and is no more relevant? If so, please close issue and PR. If not, let's chat again as I lost context.

[varunagrawal]

This is still a bug, one that I introduced accidentally.

[varunagrawal]

So I managed to figure this out after a lot of debugging.

The big issue was that when building the tree, we call Unique which does the pruning, but once the tree is built, pruned nodes are added back if the labels are not in decreasing order (this is handled by compose).

To address this, I made Unique a recursive method that now operates only after the whole tree is built and the labels reordered. The tree construction process is now: build tree bottom-up, order labels, prune top-down. The previous approach was: build and prune bottom-up, order labels.

[varunagrawal]

#1555 details some of the pending issues I have with nrAssignments. I'll get back to those after I update Hybrid to use the TableFactor.

[dellaert]

Please comment on what the status is of this PR after you merged all other 3 we discussed. Also, might want to merge in develop after that to make this PR much smaller?

[varunagrawal]

Merged in develop. The nrAssignments is still messed up due to the binary apply method, and I'll push a unit test to this PR so we can track it later.

[dellaert]

Still confused about status of this PR. Changes in 15 files, many tests now DISABLED.

[varunagrawal]

This PR is still a WIP. I'll update it as I go along.

[varunagrawal]

I removed nrAssignments since the issues go much deeper and doing a full fix would take far too long, time which I currently don't have. Also the original reason I added that feature was to improve the pruning speed, but that was accomplished in another way.

[dellaert]

OK, I like the simplification.