-
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
Wrap FindKarcherMean for Rot3 #1069
Conversation
gtsam/slam/slam.i
Outdated
@@ -323,6 +323,9 @@ virtual class KarcherMeanFactor : gtsam::NonlinearFactor { | |||
KarcherMeanFactor(const gtsam::KeyVector& keys); | |||
}; | |||
|
|||
template <T = {gtsam::Rot3}> | |||
const T FindKarcherMean<T>(const std::vector<T>& rotations); |
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.
@varunagrawal @ProfFan question for you all -- do we have support for std::vector yet? or do we need to make a new type alias here such as Rot3Vector?
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.
No because of MATLAB end
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.
huh? I thought that std::vector worked with wrapper??
gtsam/slam/slam.i
Outdated
gtsam::Rot3 at(size_t i) const; | ||
void push_back(const gtsam::Rot3& R); | ||
}; | ||
const gtsam::Rot3 FindKarcherMean<gtsam::Rot3>(const Rot3Vector& rotations); |
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.
no need for const
in the return type: gtsam::Rot3 FindKarcherMean<gtsam::Rot3>(const Rot3Vector& rotations);
python/gtsam/specializations/slam.h
Outdated
@@ -17,3 +17,4 @@ py::bind_vector< | |||
py::bind_vector< | |||
std::vector<boost::shared_ptr<gtsam::BetweenFactor<gtsam::Pose2> > > >( | |||
m_, "BetweenFactorPose2s"); | |||
py::bind_vector<std::vector<gtsam::Rot3>>(m_, "Rot3Vector"); |
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.
Are you sure you don't need aligned allocator?
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.
Thanks for the review @ProfFan and @akshay-krishnan.
Let me check about aligned allocator. Btw I don't love the name Rot3Vector
so much ... sounds like a vector representation of a Rot3. But Rot3Set
would not be consistent with Pose3Vector
nomenclature. So I guess it works.
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 minus one
gtsam/slam/slam.i
Outdated
void push_back(const gtsam::Rot3& R); | ||
}; | ||
typedef FindKarcherMean<gtsam::Rot3> FindKarcherMeanRot3; | ||
gtsam::Rot3 FindKarcherMeanRot3(const Rot3Vector& rotations); |
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.
@dellaert @ProfFan the CI states: Cannot find class FindKarcherMean in module!
. Strange. Should we be including #include <gtsam/slam/KarcherMeanFactor.h>
instead of #include <gtsam/slam/KarcherMeanFactor-inl.h>
in this .i
file?
Traceback (most recent call last):
File "/home/runner/work/gtsam/gtsam/wrap/scripts/pybind_wrap.py", line 94, in <module>
main()
File "/home/runner/work/gtsam/gtsam/wrap/scripts/pybind_wrap.py", line 85, in main
wrapper.wrap_submodule(args.src)
File "/home/runner/work/gtsam/gtsam/wrap/gtwrap/pybind_wrapper.py", line 696, in wrap_submodule
cc_content = self.wrap_file(content, module_name=module_name)
File "/home/runner/work/gtsam/gtsam/wrap/gtwrap/pybind_wrapper.py", line 626, in wrap_file
module = instantiator.instantiate_namespace(module)
File "/home/runner/work/gtsam/gtsam/wrap/gtwrap/template_instantiator/namespace.py", line 80, in instantiate_namespace
element = instantiate_namespace(element)
File "/home/runner/work/gtsam/gtsam/wrap/gtwrap/template_instantiator/namespace.py", line 59, in instantiate_namespace
typedef_inst.typename)
File "/home/runner/work/gtsam/gtsam/wrap/gtwrap/interface_parser/namespace.py", line 109, in find_class_or_function
typename.name))
ValueError: Cannot find class FindKarcherMean in module!
make[2]: *** [python/slam.cpp] Error 1
python/CMakeFiles/pybind_wrap_gtsam.dir/build.make:164: recipe for target 'python/slam.cpp' failed
CMakeFiles/Makefile2:6769: recipe for target 'python/CMakeFiles/pybind_wrap_gtsam.dir/all' failed
make[1]: *** [python/CMakeFiles/pybind_wrap_gtsam.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
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 be the simple .h file. Is the FindKarcherMean class defined in the .i file?
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.
FindKarcherMean
is not defined in the .i file yet. I should add that, no that I think about it... (btw it is a function, not a class)
For some reason the inl.h
file is being included, instead of the .h
file currently. Not sure why though
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.
Actually, since we can use std::
in .i, then we can't define FindKarcherMean in the .i
file, since it relies on std::
template <class T>
T FindKarcherMean(const std::vector<T, Eigen::aligned_allocator<T>> &rotations);
template <class T> T FindKarcherMean(std::initializer_list<T> &&rotations);
So I guess the typedef for FindKarcherMeanRot3
needs to go into the .h
file directly?
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.
But you can use std::
. Who is telling you you can't ?
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.
Matlab wrapper needs correct signature, unless @varunagrawal implemented something that handles this?
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.
If I'm not mistaken, std should be useable. When in doubt, check out or write a test for it in the wrap project. :)
(I spent a whole month writing those tests folks, please make them worth my time 😅)
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.
Matlab wrapper needs correct signature, unless @varunagrawal implemented something that handles this?
I don't know what that means. Example?
gtsam/slam/slam.i
Outdated
template <class T> | ||
T FindKarcherMean(const std::vector<T, Eigen::aligned_allocator<T>> &rotations); | ||
|
||
template <class T> T FindKarcherMean(std::initializer_list<T> &&rotations); |
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.
Not sure what this would translate to in python.....
@johnwlambert , are you planning on still submitting other commits? You should know that on every push the CI runs 34 checks. It is recommended you do everything locally, including make check and make python-install, and then push once you think everything was addressed. |
If you are planning another push, kindly cancel the CI jobs for this PR. |
I've gotten to like the Github.com interface a bit too much : - ) I've cancelled the nonrelevant CI jobs, thanks. |
Yeah, please don't develop with our CI as your compute :-) It clogs all other PRs, and I'm trying to build a wheel (which will unfortunately not include this PR). |
python/gtsam/specializations/slam.h
Outdated
@@ -17,3 +17,4 @@ py::bind_vector< | |||
py::bind_vector< | |||
std::vector<boost::shared_ptr<gtsam::BetweenFactor<gtsam::Pose2> > > >( | |||
m_, "BetweenFactorPose2s"); | |||
py::bind_vector<std::vector<gtsam::Rot3, Eigen::aligned_allocator<gtsam::Rot3>>(m_, "Rot3Vector"); |
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.
there is a missing ">" at the end here, that should let you wrap Rot3Vector at least.
It looks like we cannot use |
Thanks for taking a look @akshay-krishnan. Yeah this seems like we should just use KarcherMeanFactor directly in Python w/ an optimizer of our choice. |
Nonono! Let's fix this. Let me take a quick look. |
Why, @johnwlambert , there is not even a unit test here I can start from :-) |
@johnwlambert what were you actually trying to accomplish with this PR? There is no PR comment. And, there is |
Thanks Frank. Yeah we were just trying to avoid repeating that boilerplate from https://github.com/borglab/gtsam/blob/develop/python/gtsam/tests/test_KarcherMeanFactor.py#L24. But we can use it in Python, no problem. A simple unit test would look like import gtsam
from gtsam import Rot3, Rot3Vector
from gtsam.utils.test_case import GtsamTestCase
class TestKarcherMean(GtsamTestCase):
"""Tests for DecisionTreeFactors."""
def test_find_karcher_mean_identity() -> None:
"""Averaging 3 identity rotations should yield the identity."""
a1Rb1 = Rot3()
a2Rb2 = Rot3()
a3Rb3 = Rot3()
aRb_list = Rot3Vector([a1Rb1, a2Rb2, a3Rb3])
aRb_expected = Rot3()
aRb = gtsam.FindKarcherMeanRot3(aRb_list)
self.assertTrue(aRb.equals(aRb_expected)) but there are already some implemented here, which seem fine. |
@johnwlambert I think I fixed it, could you please update the PR with a good description after looking at the file differences? |
Beautiful work Frank. I added a bit to the description you wrote. Thanks! |
Wrap
gtsam.FindKarcherMean
withRot3Vector
argument (a new type defined in Rot3.h)Previously, only
KarcherMeanFactor
was wrapped, meaning additional boilerplate (create a factor graph, create an optimizer, and optimize it) was required to compute the Karcher Mean.Rot3Vector
is a new type that is essentially a vector ofRot3
objects, although an Eigen aligned allocator is needed.