Hybrid Bayes Net sampling #1347
Conversation
gtsam/hybrid/HybridBayesNet.cpp
Outdated
| @@ -232,6 +235,43 @@ VectorValues HybridBayesNet::optimize(const DiscreteValues &assignment) const { | |||
| return gbn.optimize(); | |||
| } | |||
|
|
|||
| /* ************************************************************************* */ | |||
| HybridValues HybridBayesNet::sample(VectorValues given, std::mt19937_64 *rng, | |||
| SharedDiagonal model) const { | |||
There was a problem hiding this comment.
Given should be const&, and model should not be there I think (per my email to you).
There was a problem hiding this comment.
Interesting. I followed the signature in the other sample methods, so maybe update there as well?
Also it can't be const since DiscreteBayesNet does a sampleInPlace(&given) and GaussianBayesNet has the line given.insert(sampled);. Would you like me to update those to make them functional?
There was a problem hiding this comment.
Oh, now I remember! It is passed by value deliberately, to allow for updating it in-place.
There was a problem hiding this comment.
Should add that in comment so we don’t forget again
There was a problem hiding this comment.
I updated GaussianBayesNet::sample to be functional. So instead of given.insert(sampled), it is now result.insert(sampled) with result initialized as VectorValues result(given);.
Perfectly backwards compatible. :)
There was a problem hiding this comment.
Yeah, not really :-) the argument was passed by value which makes a copy, functionally equivalent to what you changed it to. Modulo possible compiler optimizations that might do something different - no idea which one is best.
This reverts commit 4fc02a6.
| @@ -64,8 +64,9 @@ namespace gtsam { | |||
| return sample(result, rng); | |||
| } | |||
|
|
|||
| VectorValues GaussianBayesNet::sample(VectorValues result, | |||
| VectorValues GaussianBayesNet::sample(const VectorValues& given, | |||
There was a problem hiding this comment.
Ok, for this case, the question is whether it should be value (which will automatically make the copy you inserted below) or by const&, which you modified this code to. I chose by value when implementing this so I would get the automatic copy, but maybe it does not make a difference and it is more “clever than smart”. The code that was here was correct, though.
There was a problem hiding this comment.
I agree it was correct. I felt this made it clearer to read that you're given some VectorValues and you're getting back the result of sampling.
I can undo this if you'd like.
| @@ -79,7 +80,7 @@ namespace gtsam { | |||
| return sample(&kRandomNumberGenerator); | |||
| } | |||
|
|
|||
| VectorValues GaussianBayesNet::sample(VectorValues given) const { | |||
| VectorValues GaussianBayesNet::sample(const VectorValues& given) const { | |||
There was a problem hiding this comment.
This, for sure, is a positive change.
This PR adds a
samplemethod to theHybridBayesNet, which operates as follows:VectorValuesfrom the selected Gaussian bayes net.HybridValues.