Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper Hybrid Elimination #1319

Merged
merged 63 commits into from
Dec 30, 2022
Merged

Proper Hybrid Elimination #1319

merged 63 commits into from
Dec 30, 2022

Conversation

varunagrawal
Copy link
Collaborator

This PR adds a unit test showing that the elimination scheme isn't correct since the probabilities of the continuous variables P(X | M, Z) doesn't match the case when we individually construct each particular modality.

Basically, I have added a unit test that calls the method getSpecificProblem which returns a specific (nonlinear) factor graph corresponding to a specific discrete mode sequence. The idea is that the error of this factor graph should equal the error of the hybrid factor graph for the same discrete value assignment.

@varunagrawal varunagrawal self-assigned this Nov 1, 2022
@varunagrawal varunagrawal requested review from dellaert and ProfFan and removed request for dellaert November 1, 2022 03:36
@keevindoherty
Copy link
Contributor

Hi @varunagrawal - thanks for keeping me posted in these PRs! I had some questions related to #1318 and this PR.

I may be misunderstanding, but when I read this, I was confused about a few things:

  • I wasn't sure the relationship between the conditional p(X | M, Z) and the elimination scheme in this context. I was thinking if we eliminate discrete variables 'M' from the joint we should have p(X | Z). Are we eliminating a subset of X from the conditional? Or when you say 'elimination' here, are you referring specifically to a chordal Bayes net specifying p(X | M, Z) induced by a particular elimination order, rather than the resulting marginal?
  • Accepting for the moment that we are dealing with the conditional p(X | M, Z), it wasn't obvious to me that (the negative log of) this conditional p(X | M, Z) should correspond to the value of the hybrid factor graph error for the same discrete assignment (this certainly need not be the case in general). Specifically, we know that p(X | M = M, Z = Z) satisfies int_X p(X | M = M, Z = Z) = 1, whereas the factor graph for a specific choice of M encodes p(X, M = M, Z = Z). All we know is that this is proportional to p(X | M = M, Z = Z), but it is not necessarily normalized w.r.t. X. (I'm abusing notation a little bit here to make explicit the dependence on here on fixed assignments to M and Z, hopefully that makes sense?)

Let me know if I am missing something?

@varunagrawal
Copy link
Collaborator Author

I wasn't sure the relationship between the conditional p(X | M, Z) and the elimination scheme in this context. I was thinking if we eliminate discrete variables 'M' from the joint we should have p(X | Z). Are we eliminating a subset of X from the conditional? Or when you say 'elimination' here, are you referring specifically to a chordal Bayes net specifying p(X | M, Z) induced by a particular elimination order, rather than the resulting marginal?

Hey Kevin. Yes we eliminated as per a specific ordering (continuous followed by discrete) so we will always eliminate subsets of X before eliminating any M.

Accepting for the moment that we are dealing with the conditional p(X | M, Z), it wasn't obvious to me that (the negative log of) this conditional p(X | M, Z) should correspond to the value of the hybrid factor graph error for the same discrete assignment (this certainly need not be the case in general). Specifically, we know that p(X | M = M, Z = Z) satisfies int_X p(X | M = M, Z = Z) = 1, whereas the factor graph for a specific choice of M encodes p(X, M = M, Z = Z). All we know is that this is proportional to p(X | M = M, Z = Z), but it is not necessarily normalized w.r.t. X. (I'm abusing notation a little bit here to make explicit the dependence on here on fixed assignments to M and Z, hopefully that makes sense?)

Yes you are correct. I am using P(X | M, Z) to mean the unnormalized property, and the way I am considering it is akin to a Gaussian Mixture Model where M is our indexing variable. The idea is then that given a particular Gaussian index, we get the Gaussian P(X | Z). In fact, that is exactly what my unit test in this PR is doing, generating each Gaussian separately and verifying that elimination gives us the equivalent numbers. If I am misunderstanding your question, please let me know.

@ProfFan
Copy link
Collaborator

ProfFan commented Nov 2, 2022

I think what Kevin refers to here is that since our hybrid factors just chooses one of a bunch continuous factors, they do not necessarily have the same normalization constant thus you will have a different scale for the branches, which will make your estimation biased. Please correct me if I am speaking nonsense, @keevindoherty :)

@varunagrawal
Copy link
Collaborator Author

I agree that each leaf will have a different normalization constant, but that shouldn't make any of the leaves biased.

The way to think of this is maintaining N different chains, one corresponding to each leaf. If we perform MLE on each of the chains, we can pick the chain that is best supported by the measurement data. This is a similar idea to particle filtering, where each particle represents a particular discrete mode sequence.

Base automatically changed from hybrid/switching-0-index to varun/pruning-fix November 2, 2022 23:47
Base automatically changed from varun/pruning-fix to develop November 3, 2022 10:21
@varunagrawal varunagrawal changed the base branch from develop to hybrid/error November 4, 2022 19:21
@varunagrawal
Copy link
Collaborator Author

@ProfFan @keevindoherty @dellaert I managed to figure out the correct way to perform elimination and set the discrete probabilities such that the probability P'(Continuous | Discrete) is the same if we have M different (continuous-only) factor graphs, where M is the total number of discrete modes.

To do this, I had to create a new method eliminateHybridSequential and the resulting tests are in testHybridEstimation.cpp.

The bad news is that we will have to overload the the eliminateSequential methods in the HybridGaussianFactorGraph class, since otherwise some of the prior tests fail.

Base automatically changed from hybrid/error to develop December 23, 2022 05:22
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think once #1356 is merged and you merge in develop, taking care of conflicts, we can merge this once CI passes. Don't merge in the "Model Selection" things in yet.

@varunagrawal varunagrawal merged commit 6466b03 into develop Dec 30, 2022
@varunagrawal varunagrawal deleted the hybrid/tests branch December 30, 2022 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants