-
Notifications
You must be signed in to change notification settings - Fork 767
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
Fix warnings #657
Fix warnings #657
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.
Nice. But. Some issues, in comments.
M2 << 0, 0, 0, 0, 0, 0; | ||
values.insert(2, Vector3(0, 0, 0)); | ||
values.insert(4, Vector3(0, 0, 0)); | ||
values.insert(6, M1); |
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.
How about Z_2x3 ?
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.
Z_2x3
isn't defined but I see what you mean.
@@ -1,4 +1,4 @@ | |||
cmake_minimum_required(VERSION 2.8) |
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.
We can't make changes inside metis. Licensing and upgrade difficulties.
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.
Interesting. I saw the git-blame for this CMake file and this file has been updated before, hence I went ahead. Does this new information change anything?
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, maybe the Makefile is what we added, and we should not make changes in the library itself.
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.
Yup, the library is unchanged. 🙂 Guess we'll keep this update.
gtsam/slam/GeneralSFMFactor.h
Outdated
@@ -70,6 +70,7 @@ class GeneralSFMFactor: public NoiseModelFactor2<CAMERA, LANDMARK> { | |||
protected: | |||
|
|||
Point2 measured_; ///< the 2D measurement | |||
bool verbose_; ///< Flag for print verbosity |
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.
Adding a boolean flag for 100k factors in an SFM problem. Problematic. Propose to leave this out of this PR and discuss a better strategy
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.
Alright. Should I leave the exception printing in? Ideally that should be part of a logging framework. I'll already looked up some options.
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, no printing. Let's leave for when we have logging, which would be great. I do like glog :-)
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.
Yeah glog and CodeCov (for unit test code coverage) are two things on my radar. I'll create an issue for it that someone else can tackle.
This PR fixes a bunch of cmake and compiler warnings.