Skip to content
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

Merged
merged 7 commits into from
Jan 5, 2021
Merged

Fix warnings #657

merged 7 commits into from
Jan 5, 2021

Conversation

varunagrawal
Copy link
Collaborator

This PR fixes a bunch of cmake and compiler warnings.

@varunagrawal varunagrawal self-assigned this Jan 5, 2021
@varunagrawal varunagrawal added cmake Related to CMake and build system enhancement Improvement to GTSAM labels Jan 5, 2021
Copy link
Member

@dellaert dellaert left a 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Z_2x3 ?

Copy link
Collaborator Author

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)
Copy link
Member

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.

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Collaborator Author

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.

@@ -70,6 +70,7 @@ class GeneralSFMFactor: public NoiseModelFactor2<CAMERA, LANDMARK> {
protected:

Point2 measured_; ///< the 2D measurement
bool verbose_; ///< Flag for print verbosity
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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 :-)

Copy link
Collaborator Author

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.

@varunagrawal varunagrawal merged commit 745e220 into develop Jan 5, 2021
@varunagrawal varunagrawal deleted the fix/compiler-warnings branch January 5, 2021 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Related to CMake and build system enhancement Improvement to GTSAM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants