Improved HybridBayesNet API

[varunagrawal]

1. Support adding both pointer and non-pointer types to the HybridBayesNet.
2. Break up HybridValues into .h and .cpp.
3. Update tests.

[dellaert]

Hmmm. If anything I want to get away from adding non-pointer types to FG/BN. There is just a useless copy and another allocation. The preferred style to promote is push_pack_shared<T> or whatever it is called in the base factor graph class. The only reason const& versions still exist right now is backwards compatibility- I’d rather change all examples and tests to use the different style.

[varunagrawal]

I don't see a push_back_shared method, just an emplace_shared.

My main motivation for this was to be able to add shared pointers to the hybrid Bayes net. Currently we can only add raw pointers.

Adding support for non-pointer types is syntactic sugar, which makes tests less verbose. My hope is that the user is cognizant of the difference.

I'll remove the const& on the shared pointer version and attempt to add emplace_shared as an additional method. Would that work? Or do you want me to get rid of the non pointer version altogether?

[dellaert]

The only things that should be possible going forward are

• push_back a shared pointer
• emplace_shared which creates and pushes back

[varunagrawal]

@dellaert clang is having a hard time figuring out the constructor for the shared pointer if I use ({z}, {}, {m}, {c0, c1}). It's so weird.

[varunagrawal]

This seems like it is an issue regarding lvalues.

[varunagrawal]

The only things that should be possible going forward are

• push_back a shared pointer
• emplace_shared which creates and pushes back

Okay done.

[dellaert]

Hmmm, no, emplace_shared should be preferred. I don’t think this raw pointer call fits anything we have ever done in GTSAM.

[varunagrawal]

This was pre-existing and confusing to me as well. I'm going to remove it.

[dellaert]

The only things that should be possible going forward are

• push_back a shared pointer
• emplace_shared which creates and pushes back

Okay done.

I don’t see that. Can you check emplace_shared in FactorGraph.h ?

[dellaert]

This is good. Although check what we do in GaussianBayesNet. I think we use const& to a shared pointer there. My comment before about const& was about a ref to the underlying object, not the shared pointer.

[varunagrawal]

GaussianBayesNet inherits from FactorGraph which has this method (which I followed), so no const& for the shared pointer:

/// Add a factor directly using a shared_ptr.
template <class DERIVEDFACTOR>
IsDerived<DERIVEDFACTOR> push_back(std::shared_ptr<DERIVEDFACTOR> factor) {
  factors_.push_back(std::shared_ptr<FACTOR>(factor));
}

I should add const& since it provides a small performance benefit.

[dellaert]

Great!!!