-
Notifications
You must be signed in to change notification settings - Fork 779
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
Added convenience constructors and python wrappers #1353
Conversation
@@ -105,14 +104,29 @@ class HybridBayesTree { | |||
gtsam::DefaultKeyFormatter) const; | |||
}; | |||
|
|||
#include <gtsam/hybrid/HybridBayesTree.h> |
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.
Umm why are we including this here (on HybridBayesNet) rather than before HybridBayesTree?
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.
Either way it doesn't matter because the wrapper will bubble it to the top of the generated files, but still seems like a weird thing to do (especially for wrapper noobs).
python/gtsam/preamble/hybrid.h
Outdated
@@ -19,3 +19,4 @@ PYBIND11_MAKE_OPAQUE(std::vector<gtsam::Key>); | |||
#endif | |||
|
|||
PYBIND11_MAKE_OPAQUE(std::vector<gtsam::GaussianFactor::shared_ptr>); | |||
PYBIND11_MAKE_OPAQUE(std::vector<gtsam::GaussianConditional::shared_ptr>); |
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.
Why do we need this when pybind11/stl.h
is included?
Bunch of issues:
I'm pushing the updates for your convenience. |
Also, you can wrap shared pointers as Ref: wrap/DOCS.md |
|
||
conditionalProbability = gc.evaluate(values.continuous()) | ||
mixtureProbability = conditional0.evaluate(values.continuous()) | ||
assert self.assertAlmostEqual(conditionalProbability * |
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.
FYI there's a double assert here. This will always fails because self.assertAlmostEqual
returns None.
@varunagrawal thanks for the fixes. |
I don't delete the branches since you asked me not to a while back. |
@varunagrawal Late now, will you take over and merge this branch? Seems one CI fails. with this wrapper should be easier to do notebook we discussed. |
@dellaert yup I'm currently working on fixing this. Will get it done in a bit. |
I added some convenience constructors and python wrappers. However, I can't get the wrapping for
GaussianMixture.FromConditionals
to work. @ProfFan or @varunagrawal any ideas? I get the error below: