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

Added convenience constructors and python wrappers #1353

Merged
merged 11 commits into from
Dec 29, 2022

Conversation

dellaert
Copy link
Member

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:

  File "/Users/dellaert/git/github/python/gtsam/tests/test_HybridBayesNet.py", line 44, in test_evaluate
    gm = GaussianMixture.FromConditionals([X(1)], [], [Asia], [conditional0, conditional1])  #
TypeError: FromConditionals(): incompatible function arguments. The following argument types are supported:
    1. (continuousFrontals: std::__1::vector<unsigned long long, std::__1::allocator<unsigned long long> >, continuousParents: std::__1::vector<unsigned long long, std::__1::allocator<unsigned long long> >, discreteParents: gtsam.gtsam.DiscreteKeys, conditionalsList: std::__1::vector<boost::shared_ptr<gtsam::GaussianConditional>, std::__1::allocator<boost::shared_ptr<gtsam::GaussianConditional> > >) -> gtsam.gtsam.GaussianMixture

Invoked with: [8646911284551352321], [], [(6989586621679009792, 2)], [GaussianConditional p(x1)
  R = [ 1 ]
  d = [ 5 ]
isotropic dim=1 sigma=2
, GaussianConditional p(x1)
  R = [ 1 ]
  d = [ 2 ]
isotropic dim=1 sigma=3
]

Did you forget to `#include <pybind11/stl.h>`? Or <pybind11/complex.h>,
<pybind11/functional.h>, <pybind11/chrono.h>, etc. Some automatic
conversions are optional and require extra headers to be included
when compiling your pybind11 module.

@dellaert dellaert self-assigned this Dec 29, 2022
@@ -105,14 +104,29 @@ class HybridBayesTree {
gtsam::DefaultKeyFormatter) const;
};

#include <gtsam/hybrid/HybridBayesTree.h>
Copy link
Collaborator

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?

Copy link
Collaborator

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).

@@ -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>);
Copy link
Collaborator

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?

@varunagrawal
Copy link
Collaborator

Bunch of issues:

  1. You don't need the extra PYBIND11_MAKE_OPAQUE(std::vector<gtsam::Key>); in hybrid.h. That's already defined once in gtsam.h for the wrapper. The second one is causing a conflict for pybind and hence the issue you're facing.
  2. You need to pass in a gtsam.DiscreteKeys, not a List[int, int] for the DiscreteKeys argument.

I'm pushing the updates for your convenience.

@varunagrawal
Copy link
Collaborator

Also, you can wrap shared pointers as std::vector<gtsam::GaussianConditional*> as well as std::vector<gtsam::GaussianConditional::shared_ptr>. You may have forgotten I added this functionality as per your specification. 🙂

Ref: wrap/DOCS.md


conditionalProbability = gc.evaluate(values.continuous())
mixtureProbability = conditional0.evaluate(values.continuous())
assert self.assertAlmostEqual(conditionalProbability *
Copy link
Collaborator

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.

@dellaert
Copy link
Member Author

@varunagrawal thanks for the fixes.
if you’re good, Feel free to approve and merge to develop (don’t understand why base branch did not automatically change to develop).

@varunagrawal
Copy link
Collaborator

don’t understand why base branch did not automatically change to develop

I don't delete the branches since you asked me not to a while back.

Base automatically changed from feature/HBN-evaluate to develop December 29, 2022 03:50
@dellaert
Copy link
Member Author

@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.
Talk to you tomorrow.

@varunagrawal
Copy link
Collaborator

@dellaert yup I'm currently working on fixing this. Will get it done in a bit.

@varunagrawal varunagrawal merged commit 706a8a4 into develop Dec 29, 2022
@varunagrawal varunagrawal deleted the feature/evaluate_wrappers branch December 29, 2022 08:23
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.

3 participants