Add python bindings for FloatingBaseEstimator and LeggedOdometry #218
Conversation
There was a problem hiding this comment.
Great work @prashanthr05, looks good to me apart minor nits. A simple example / test would be highly appreciated, especially to provide users a basic way to configure these newly exposed classes.
| // at the moment at an interface level, this nested class need not be exposed | ||
| // but in case, we want to use the FloatingBaseEstimator as base class | ||
| // for python based implementations, maybe it could be useful |
There was a problem hiding this comment.
This is definitely possible with pybind, but it requires quite a lot of work since it demands creating a dummy class (called trampoline) for the interface. In my opinion, once the entire infrastructure is up and running, this could be a great alternative for prototyping, but as you wrote, let's keep this option for future work.
|
|
||
| py::class_<FloatingBaseEstimator, Advanceable<Output>> floatingBaseEstimator(module, "FloatingBaseEstimator"); | ||
|
|
||
| floatingBaseEstimator.def(py::init()) |
There was a problem hiding this comment.
Any reason to separate methods definition from the creation of the py::class_ object?
There was a problem hiding this comment.
It was suggested as a way to handle nested classes or internal types.
I followed examples suggested in,
There was a problem hiding this comment.
Ok, only now I see that the object is used below, good to know!
| .def("get", &FloatingBaseEstimator::get) | ||
| .def("is_valid", &FloatingBaseEstimator::isValid) | ||
| .def("initialize", | ||
| [](FloatingBaseEstimator& impl, std::shared_ptr<IParametersHandler> handler, std::shared_ptr<iDynTree::KinDynComputations> kinDyn) -> bool { |
There was a problem hiding this comment.
I think these files are not formatted with project's clang-format. This line for instance seems way too long.
@GiulioRomualdi I see this in many files of blf. Do you think that a new specific PR that uniforms all the files of this project (and then enforcing it) could be done? In order to minimize conflicts, maybe we can do it in a moment with no open PRs?
There was a problem hiding this comment.
Thanks @GiulioRomualdi for taking care of the formatting in 0a86455
Yes, I am going to prepare a test script for that. |
I have added an interface test describing how to run the estimator in this commit c515ba4 |
|
Thank you all for the effort 😄 |
Testing is pending.