Direct Hybrid Factor Specification #1805
Conversation
|
I’d like to chat about this more. While the HBN -> HFG is now very clear, the proposed API here is not. Where do the log normalizers come from? What is their correct value? What do they mean? If the answer is: “think about the conditional and its normalizer” then why not use the “likelihood” approach, or just directly add a Hybrid conditional? Let’s chat in person because I think it requires higher bandwidth than comments back and forth… |
|
|
||
| // Create HybridGaussianFactor | ||
| std::vector<GaussianFactorValuePair> factors{ | ||
| {f0, ComputeLogNormalizer(model0)}, {f1, ComputeLogNormalizer(model1)}}; |
There was a problem hiding this comment.
I'm still very shaky about the semantics of this scalar. I asked a question on the previous PR but I don;t think it was answered. If the scalar simply adds to the error, then is that after squaring and halving? If so, I think this needs to be better documented, with an example in the .h file even. There is also uncertainty about the sign: is the normalizer multiplying or dividing? I think the former, but again: be very clear, also in the docs of ComputeLogNormalizer. And, very important, it has to be consistent everywhere.
There was a problem hiding this comment.
I sent you the doc with the math written up there. From the derivation, we are adding the pre-halved version. You are right that it should be better documented in the .h file. :)
ComputeLogNormalizer returns
There was a problem hiding this comment.
OK, this answer is problematic to me: when specifying the API for hybrid factor graphs, the normalizer is just an application. Ideally, the API in the .h file is documented like:
In factor graphs the error function typically returns 0.5*|h(x)-z|^2, i.e., the negative log-likelihood for a Gaussian noise model. In hybrid factor graphs we allow *adding* an arbitrary scalar dependent on the discrete assignment. For example, adding a 70/30 mode probability is supported by providing the scalars $-log(.7)$ and $-log(.3)$. Note that adding a common constant will not make any difference in the optimization, so $-log(70)$ and $-log(30)$ work just as well.
Please consider adding the above in HNF and the linear version in HGF. And, not that I am advocating here for adding these scalars terms to the error. When designing the API for the factors, it does not make sense to tell people that a term will be subtracted: we either work in negative log space (we add) or in probability space (we multiply). I see no need to add subtraction into this lexicon.
There was a problem hiding this comment.
Happy to chat in person BTW. Invite me.
There was a problem hiding this comment.
As discussed, I have made all the changes.
|
Please update PR comment? |
… since it gets cancelled out during normalization
gtsam/hybrid/HybridGaussianFactor.h
Outdated
| @@ -45,6 +45,15 @@ using GaussianFactorValuePair = std::pair<GaussianFactor::shared_ptr, double>; | |||
| * where the set of discrete variables indexes to | |||
| * the continuous gaussian distribution. | |||
| * | |||
| * In factor graphs the error function typically returns 0.5*|h(x)-z|^2, i.e., | |||
There was a problem hiding this comment.
Very close, but still one main issue:
- I think the factor 2 is still in the wrong place for the GaussianHybridFactor
Two possibly significant performance issues:
- unzipping and then undoing the unzip is needlessly expensive.
- only having the Gaussian logNormalization constant might be very expensive.
If you want I can create a PR with the noiseModel changes if you finish the other things.
| @@ -56,7 +56,7 @@ HybridGaussianFactor::Factors augment( | |||
| const HybridGaussianFactor::sharedFactor &gf) { | |||
| auto jf = std::dynamic_pointer_cast<JacobianFactor>(gf); | |||
| if (!jf) return gf; | |||
| // If the log_normalizer is 0, do nothing | |||
| // If the value is 0, do nothing | |||
| if (values(assignment) == 0.0) return gf; | |||
There was a problem hiding this comment.
You are calling the potentially expensive values(assignment) twice, so please assign it to a variable. But why unzip the tree and then call values() at all. Can't you just apply on the original tree and get the value for free?
A more problematic issue is that you take the square root. But is this wrong now? Should we not do sqrt(2*value) ? Because the "hidden" value c will be squared and halved: value == 0.5*c^2.
There was a problem hiding this comment.
-
So I need to unzip to normalize the values (so that we don't have negatives, else the sqrt(value) gives a nan). This brings up an interesting point though: should we normalize for the user or should the user pre-normalize? I felt like us doing it makes sense, since the sqrt to hide the variable is something we need to do and the user shouldn't have to worry about those semantics. Thoughts?
-
We can do 2*value, but that is going to have to also change in various other places like
HybridGaussianConditional::likelihood. I think what you suggest is the better thing, keeping in alignment with my previous point.
…eterminant and logNormalizationConstant on top of it.
Add tests which illustrate how we can specify hybrid factors directly instead of defining the BayesNet and calling
toFactorGraphon it.This is useful since it allows us to handle nonlinear functions in the conditionals being converted to factors. This will also help with relinearization, a long standing issue for us in this scheme.