-
Notifications
You must be signed in to change notification settings - Fork 795
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
Changes from 4 commits
02abc53
a650a6f
473a6a1
b7584ce
b244a7d
5b52e4c
0f03ee1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -175,10 +175,13 @@ TEST(Values, basic_functions) | |
{ | ||
Values values; | ||
const Values& values_c = values; | ||
values.insert(2, Vector3()); | ||
values.insert(4, Vector3()); | ||
values.insert(6, Matrix23()); | ||
values.insert(8, Matrix23()); | ||
Matrix23 M1, M2; | ||
M1 << 0, 0, 0, 0, 0, 0; | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
values.insert(8, M2); | ||
|
||
// find | ||
EXPECT_LONGS_EQUAL(4, values.find(4)->key); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
|
||
public: | ||
|
||
|
@@ -86,12 +87,17 @@ class GeneralSFMFactor: public NoiseModelFactor2<CAMERA, LANDMARK> { | |
* @param cameraKey is the index of the camera | ||
* @param landmarkKey is the index of the landmark | ||
*/ | ||
GeneralSFMFactor(const Point2& measured, const SharedNoiseModel& model, Key cameraKey, Key landmarkKey) : | ||
Base(model, cameraKey, landmarkKey), measured_(measured) {} | ||
|
||
GeneralSFMFactor():measured_(0.0,0.0) {} ///< default constructor | ||
GeneralSFMFactor(const Point2 & p):measured_(p) {} ///< constructor that takes a Point2 | ||
GeneralSFMFactor(double x, double y):measured_(x,y) {} ///< constructor that takes doubles x,y to make a Point2 | ||
GeneralSFMFactor(const Point2& measured, const SharedNoiseModel& model, | ||
Key cameraKey, Key landmarkKey, bool verbose = false) | ||
: Base(model, cameraKey, landmarkKey), | ||
measured_(measured), | ||
verbose_(verbose) {} | ||
|
||
GeneralSFMFactor() : measured_(0.0, 0.0) {} ///< default constructor | ||
///< constructor that takes a Point2 | ||
GeneralSFMFactor(const Point2& p) : measured_(p) {} | ||
///< constructor that takes doubles x,y to make a Point2 | ||
GeneralSFMFactor(double x, double y) : measured_(x, y) {} | ||
|
||
virtual ~GeneralSFMFactor() {} ///< destructor | ||
|
||
|
@@ -127,7 +133,9 @@ class GeneralSFMFactor: public NoiseModelFactor2<CAMERA, LANDMARK> { | |
catch( CheiralityException& e) { | ||
if (H1) *H1 = JacobianC::Zero(); | ||
if (H2) *H2 = JacobianL::Zero(); | ||
// TODO warn if verbose output asked for | ||
if (verbose_) { | ||
std::cout << e.what() << std::endl; | ||
} | ||
return Z_2x1; | ||
} | ||
} | ||
|
@@ -149,7 +157,9 @@ class GeneralSFMFactor: public NoiseModelFactor2<CAMERA, LANDMARK> { | |
H1.setZero(); | ||
H2.setZero(); | ||
b.setZero(); | ||
// TODO warn if verbose output asked for | ||
if (verbose_) { | ||
std::cout << e.what() << std::endl; | ||
} | ||
} | ||
|
||
// Whiten the system if needed | ||
|
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.