-
Notifications
You must be signed in to change notification settings - Fork 779
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
Moved Sfm classes to their own files #1078
Conversation
dellaert
commented
Jan 31, 2022
- Moved SfmData and SfmTrack into sfm
- deprecated incorrectly named functions
- moved wrapping
bool equals(const SfmTrack& sfmTrack, double tol = 1e-9) const; | ||
|
||
/// @} | ||
#ifdef GTSAM_ALLOW_DEPRECATED_SINCE_V42 |
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.
Hi Frank, thanks for the PR. Was curious how long we plan to keep this flag set to true by default (GTSAM_ALLOW_DEPRECATED_SINCE_V42
).
We do use the number_measurements()
flag and number_tracks()
methods all over GTSFM, so we would need to refactor quite a few files. Was just curious the rationale for the renaming.
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.
Flag should actually be OFF by default.
Yes, I'm aware that if we build a new wheel (4.2a5) you'll have to amend GTSAM.
Looks like CI is failing:
|
I know, I'll fix it, along with addressing any other comments you might have. If you have no comments, please approve. If you do, still approve and trust me I will execute on them or talk to you if not :-) |
Sounds good @dellaert. The PR looks nice and I'll approve on my end. I like the decomposition of the BAL stuff into sfmData.cpp, since it never belonged in slamDataset.cpp so much. I understand the snake case -> camel case change, to be consistent with our C++ guidelines. But I personally would find |
yeah, I could change that... |