Big Re-"Factor" of Hybrid

[dellaert]

• made Factor the common base class
• this allowed removing the (cumbersome) wrapper classes

Most changes are in HybridGaussianFactorGraph, and the rest is mainly removing files and boilerplate. We also make good use now of emplace_shared.

The dispatch is ugly and possibly slower than vtables (although probably negligible?), so I am also thinking about streamlining even more

[varunagrawal]

I'm going to need some time to look into this in detail. A bit lost since I thought we need those wrapper classes for type erasure so whatever sorcery happened to eliminate that needs to be well understood for me.

[dellaert]

I'm going to need some time to look into this in detail. A bit lost since I thought we need those wrapper classes for type erasure so whatever sorcery happened to eliminate that needs to be well understood for me.

It was actually very simple: a HybridFactorGraph is now simply a FG of Factors. You can stick whatever Factor type in there - the idea being that in GTSAM 5 the most general FG is “hybrid”. In the new scheme, the HybridFactor class is just the base class for factors that actually have both discrete and continuous arguments.

The (annoying, boilerplate generating) sorcery was only needed for our older factors to “pretend” to be Hybrid.

Please don’t take too much time? I want to finish this and try to add an error function in the Factor base class in a next PR. And, I also know how we can get rid of the GraphAndConstant thing.

[dellaert]

Just removed remaining emplace_discrete and emplace_hybrid, which are now redundant: emplace_shared can be used for all factor types.

[varunagrawal]

To be honest, I am not a fan of this update. We're sacrificing a lot of readability and code-convenience just for the sake of removing a few "add/insert" methods which seems like a skewed pros-to-cons ratio.

I think the correct way forward that helps maintain all the readability is to make HybridFactor a superclass of DiscreteFactor, GaussianFactor and NonlinearFactor (basically every factor that the HybridFactorGraph is capable of handling currently). This will result in clean code but with the desired API design.

This is important going forward since if Hybrid is to be the de-facto mechanism, we don't want contributors balking at the current complexity and difficulty in parsing the code (especially if they're not proficient in C++ at your level, which is pretty darn high).

[varunagrawal]

Unattributed comment?
I believe the comment is misleading. It should be

// Encountered a BayesTreeOrphanWrapper in continuous elimination, so throw error.

Unless I am missing something.

[dellaert]

Normally, TODOs come with a username so we can ask who wrote the comment. I have no idea whether something needs to happen here or not, or it should be ignored.

[dellaert]

To be honest, I am not a fan of this update. We're sacrificing a lot of readability and code-convenience just for the sake of removing a few "add/insert" methods which seems like a skewed pros-to-cons ratio.

I disagree. I think everything become much simpler, and simpler to understand. The new style: just use emplace_shared. It names the thing it is inserting, and avoids a copy. Examples abound in the unit tests, which is what people copy, anyway. The previous code was baroque.

[varunagrawal]

I disagree. I think everything become much simpler, and simpler to understand. The new style: just use emplace_shared. It names the thing it is inserting, and avoids a copy. Examples abound in the unit tests, which is what people copy, anyway. The previous code was baroque.

I agree there was a lot of extra fat in the previous code, and this PR is a wonderful for addressing that! But all the pointer up- and down-casting is what I am referring to as being really confusing and less readable, while there exists an alternative that gives us the best of both worlds.

[dellaert]

I disagree. I think everything become much simpler, and simpler to understand. The new style: just use emplace_shared. It names the thing it is inserting, and avoids a copy. Examples abound in the unit tests, which is what people copy, anyway. The previous code was baroque.

I agree there was a lot of extra fat in the previous code, and this PR is a wonderful for addressing that! But all the pointer up- and down-casting is what I am referring to as being really confusing and less readable, while there exists an alternative that gives us the best of both worlds.

I hear you. But (a) that code is mostly internal and pretty localized, (b) I have a follow-up PR that at least removes some of the up-casting. Note virtual methods are just cleaner upcasting.