Conversation
|
@varunagrawal FYI |
There was a problem hiding this comment.
Okay this PR is a very nice effort, but I am not comfortable with the wrapping for 2 reasons:
- Lack of tests: whatever I wrapped, I extensively unit tested, and my own work allowed for integration testing.
- Verification with the Matlab wrapper. The matlab wrapper is more finicky than the python wrapper since it cares a whole lot more about return types and other smaller details. I don't see if this was checked (CI doesn't do it).
Moreover, I want to land other hybrid related PRs first (such as DCSAM) and include a new class called Key[Algebraic]DecisionTree which specializes AlgebraicDecisionTree<Key> to make its use much easier and cleaner. It's a low priority item since it simplifies the API but is a lot of effort for a single developer (aka me).
I am not selecting Request changes (which would be the right way to go), but I DO NOT recommend merging this yet.
| template <> | ||
| struct traits<HybridFactor> : public Testable<HybridFactor> {}; | ||
|
|
||
| // For wrapper: |
There was a problem hiding this comment.
The reason I haven't wrapped a lot of stuff in hybrid is primarily because of this class, and also because I want to add a new class called KeyDecisionTree which specializes the template AlgebraicDecisionTree.
This would have many benefits since we wouldn't have to always pass in keyFormatter arguments, and wrapping would be a cinch, but it is a big PR which is lower priority than some others (such as DCSAM).
| const gtsam::VectorValues& frontals) const; | ||
| double logProbability(const gtsam::HybridValues& values) const; | ||
| double evaluate(const gtsam::HybridValues& values) const; | ||
| // double operator()(const gtsam::HybridValues &values) const; |
There was a problem hiding this comment.
This should just work right?
There was a problem hiding this comment.
I tried this but I am getting an error from Pybind11. This is due to the fact that there are two operator() methods (i.e. it is overloaded), and the binding code generated is
.def("__call__", >sam::HybridGaussianConditional::operator());which cannot disambiguate between the two operator() overloads in HybridGaussianConditional since there is no specification of the argument types. There is a way to fix this but it will need more wrapper updates, so commenting this out until then.
varunagrawal
left a comment
varunagrawal
left a comment
There was a problem hiding this comment.
Please verify the matlab wrapper compiles and update the restrict method.
|
Couple of things:
|
varunagrawal
left a comment
varunagrawal
left a comment
There was a problem hiding this comment.
Fixed everything so this should be good to go. :)
|
@dellaert can you take one more look at this to make sure it matches your expectations? There were some merge conflicts which I fixed, so wanted to get your sign off on those. |
| const gtsam::VectorValues& frontals) const; | ||
| double logProbability(const gtsam::HybridValues& values) const; | ||
| double evaluate(const gtsam::HybridValues& values) const; | ||
| // double operator()(const gtsam::HybridValues &values) const; |
3 participants
New docs for hybrid folder. There were a lot of classes in here that had many, many functions that were not yet wrapped. Frank also exposed
AlgebraicDecisionTreeKeyfor methods like errorTree().Finished Notebooks