-
Notifications
You must be signed in to change notification settings - Fork 768
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
Remove boost from TableFactor and added guards to testSerializationSlam #1552
Remove boost from TableFactor and added guards to testSerializationSlam #1552
Conversation
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.
Looks good but I believe the guard in the test file should not be needed.
tests/testSerializationSlam.cpp
Outdated
@@ -52,6 +52,8 @@ | |||
#include <gtsam/base/serializationTestHelpers.h> | |||
#include <gtsam/base/std_optional_serialization.h> | |||
|
|||
#ifdef GTSAM_USE_BOOST_FEATURES |
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.
I think this can be removed since we exclude this test file using CMake if boost serialization is disabled.
Ref: tests/CMakeLists.txt
Does this test file still get compiled even with the boost serialization flag set to OFF?
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.
Yes, with both USE_BOOST_FEATURES and ENABLE_BOOST_SERIALIZATION set to OFF, it still attempts to compile this test.
I think I see the problem. the CMakeLists.txt tries to exclude "testSerializationSLAM.cpp" (with SLAM in all caps) instead of "testSerializationSlam.cpp"
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.
That seems plausible. I can't reproduce the issue on my end but that may be because of differences in CMake versions or the platform (I'm using MacOS).
Can you please update the CMakeLists.txt file? Then we can remove this guard.
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.
OK, I have update the CMakeLists.txt file and removed the guard, and now it works on my end.
@@ -203,7 +202,7 @@ void TableFactor::print(const string& s, const KeyFormatter& formatter) const { | |||
cout << s; | |||
cout << " f["; | |||
for (auto&& key : keys()) | |||
cout << boost::format(" (%1%,%2%),") % formatter(key) % cardinality(key); | |||
cout << " (" << formatter(key) << "," << cardinality(key) << "),"; |
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.
This looks great. I can verify the printing is the same as boost.
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.
Awesome sauce
This PR removes the dependence on boost from the recently added
TableFactor
, and adds some guards totestSerializationSlam
if boost is disabled. These were currently the only two things preventing gtsam from building and passing all unit tests without boost.I attempted to maintain the style of the output in TableFactor, but I didn't explicitly test with boost to compare.