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

Add python equivalent for SFMExample_bal.cpp #556

Merged
merged 12 commits into from
Oct 19, 2020

Conversation

johnwlambert
Copy link
Contributor

@johnwlambert johnwlambert commented Oct 5, 2020

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:

A later PR should include

  • SfmData -> set up of collection is considered part of “initialization”. Can add .push_back() method to this struct, Unit tests of newly wrapped functionality

@johnwlambert johnwlambert changed the title WIP (do not merge) Add python equivalent for SFMExample_bal.cpp Add python equivalent for SFMExample_bal.cpp (do not merge yet) Oct 10, 2020
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 please don’t do the SfMCamera name change

gtsam/gtsam.i Outdated Show resolved Hide resolved

import gtsam
from gtsam import (
GeneralSFMFactorSfmCamera,
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python/gtsam/examples/SFMExample_bal.py Show resolved Hide resolved
python/gtsam/examples/SFMExample_bal.py Outdated Show resolved Hide resolved
@johnwlambert
Copy link
Contributor Author

@dellaert Hi Frank, should be ready for a re-review now. Thanks!

@johnwlambert johnwlambert changed the title Add python equivalent for SFMExample_bal.cpp (do not merge yet) Add python equivalent for SFMExample_bal.cpp Oct 14, 2020
@johnwlambert
Copy link
Contributor Author

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.

Copy link
Contributor

@swarrier246 swarrier246 left a 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)
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

@johnwlambert
Copy link
Contributor Author

johnwlambert commented Oct 19, 2020

@swarrier246 thanks for the review, just added an extended comment about what the BAL contents look like

@dellaert dellaert merged commit 5adf4dc into borglab:develop Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants