-
Notifications
You must be signed in to change notification settings - Fork 769
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
Add python equivalent for SFMExample_bal.cpp #556
Add python equivalent for SFMExample_bal.cpp #556
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 please don’t do the SfMCamera name change
|
||
import gtsam | ||
from gtsam import ( | ||
GeneralSFMFactorSfmCamera, |
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.
Please format entire file with google style
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'm following black
here for the multi-line imports. Is there a particular PEP number you could point me to for Google style?
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.
@dellaert Hi Frank, should be ready for a re-review now. Thanks! |
…ython_sfm_example_bal
Hi @dellaert @akshay-krishnan, can we go ahead and merge this today? @xwu4lab @swarrier246, it might be helpful for you all to see the factors we use here as well. |
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.
would it be helpful to mention what the input file contents and format will be?
input_file = gtsam.findExampleDataFile(args.input_file) | ||
|
||
# Load the SfM data from file | ||
mydata = readBal(input_file) |
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 it would be nice to rename mydata to something more descriptive of the type of data.
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 scene_data
@akshay-krishnan ?
@swarrier246 thanks for the review, just added an extended comment about what the |
Adds Bundle Adjustment functionality to the Python wrapper.
Should provide the equivalent of the C++ example (https://github.com/borglab/gtsam/blob/develop/examples/SFMExample_bal.cpp)
Present ambiguity @dellaert -- is
typedef gtsam::PinholeCamera<gtsam::Cal3Bundler> PinholeCameraCal3Bundler;
or
typedef gtsam::PinholeCamera<gtsam::Cal3Bundler> SfmCamera;
preferred? We use both throughout codebase currently.
Introduces:
See Line 34 and Line 68 of https://github.com/borglab/gtsam/blob/develop/examples/SFMExample_bal.cpp
A later PR should include