-
Notifications
You must be signed in to change notification settings - Fork 765
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
Linear HybridBayesNet optimization #1270
Conversation
@xsj01 can you please run a formatter on your changed code? That should make reading and reviewing it easier. |
* is typically used to store the variables of a HybridGaussianFactorGraph. | ||
* Optimizing a HybridGaussianBayesNet returns this class. | ||
*/ | ||
class GTSAM_EXPORT HybridValues { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for thoughts: I always wondered can we just get rid of DiscreteKey
and stuff, and store the cardinality of variables somewhere else, like along with the value itself.
DiscreteValues discrete; | ||
|
||
// VectorValue stored the continuous components of the HybridValues. | ||
VectorValues continuous; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name the class HybridVectorValues
? This class is linear only. (comment only)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I can change VectorValues to Values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is more like a design question: Is this limited to linear hybrid systems? If so we should name it Gaussian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will extend that for nonlinear hybrid later, but currently it's only for linear hybrid system.
If we rename this to HybridVectorValues, then we will also need HybridValues for nonlinear hybrid system later. Is it necessary to have both HybridVectorValues and HybridValues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the spirit for linear
and nonlinear
though. because VectorValues and values are different we should probably have the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tested locally, seems to be working
I think this was merged too soon. I didn't get to take a good look at it. :( |
We can change this later, btw @xsj01 there is also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts and questions. Unfortunately this PR was merged too soon. 😞
HybridLookupTable(HybridConditional& conditional) : Base(conditional){}; | ||
|
||
/** | ||
* @brief Calculate assignment for frontal variables that maximizes value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xsj01 What would be the frontal variables here? They should all be continuous variables since we eliminate those first, so then this doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems the HybridLookupTable is not necessary. Just had some discussions with Fan, I will submit a new PR to fix this.
* is typically used to store the variables of a HybridGaussianFactorGraph. | ||
* Optimizing a HybridGaussianBayesNet returns this class. | ||
*/ | ||
class GTSAM_EXPORT HybridValues { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the use of this class. Can't we simply have optimize
run MPE on the discrete tree and then run a regular optimization on the continuous values corresponding to the assignment? That way we can just return a pair<Values, DiscreteValues>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that's also an option. But I feel like the api is not very user friendly while accessing/assigning values. We can talk about it in our meeting.
|
||
/** A DAG made from hybrid lookup tables, as defined above. Similar to | ||
* DiscreteLookupDAG */ | ||
class GTSAM_EXPORT HybridLookupDAG : public BayesNet<HybridLookupTable> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for this class are severely lacking. @xsj01 you have to be better than just copy-pasting the same docstrings, since now the motivation for this class is lost.
Not quite the point. There has to be coherence so that everyone working on hybrid estimation understands the overall design and there is less wasted effort. Going back and forth won't solve issues with design. I had requested a PR from @xsj01 so I could see his work and understand his requirements, with the hope that it would lead to better overall structure. |
Also the tests don't have any docs. :( |
@xsj01 can you please respond with a plan to tackle these issues? |
Sorry for the delay @varunagrawal, I will address the comments. Just wondering shall we revert this PR and merge it later? |
This PR adds the following: