-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Modernized bundle adjustment interface #2896
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
Conversation
edited
- Defines a single consistent abstract interface for bundle adjustment.
- Reuses functionality between different bundle adjusters using composition rather than inheritance.
- Extracts the pose prior alignment to the alignment module. Ideally needs tests but this remains a TODO.
- The problem setup is now performed when the bundle adjuster is created and, as such, disentangled from the solving of the problem. This renders the current awkward SetUpProblem unnecessary.
- The idea later is then to pass a bundle adjuster object to a simplified covariance estimation interface.
Thanks! The general idea is fantastic. Will try to find time later this week to look into details. |
Thanks. Looks great! The pybind here needs to be updated: https://github.com/colmap/colmap/blob/main/src/pycolmap/estimators/bundle_adjustment.cc#L144-L164. Also the example runner here: https://github.com/colmap/colmap/blob/main/pycolmap/examples/custom_bundle_adjustment.py#L196-L208 |
@B1ueber2y This should be ready for a final review. Thanks. |
Thanks. Should we update the runner here as well: https://github.com/colmap/colmap/blob/main/pycolmap/examples/custom_bundle_adjustment.py#L196-L208? I expect this part of code to be broken with the new interface? |
@B1ueber2y I fixed the custom_bundle_adjustment.py code with the new interface. The idea is that the bundle adjustment problem is configured purely through the config object instead of providing access to the internal methods. One still has full control over adding custom residuals to the existing problem. I verified that this works end-to-end together with the custom_incremental_mapper.py (for which I added the option to directly call it from the command-line to process a dataset). |
The TypeError: Unable to convert function return value to a Python type! The signature was So I added "import pyceres" back to avoid confusion when the user tries the code without pyceres installed. |
Also It might be helpful for stabilizing pycolmap interfaces if we added the example script (and potentially other python tests) to the CI pipeline. |
Yes, agreed. On my TODO list. My idea would be to expose the colmap/scene/synthetic.h interface, so we can have equivalent end-to-end mapping tests as we have under colmap/controllers/incremental_pipeline_test.cc. Later, the synthetics also come in handy for implementing tests for other parts of the python interface. |
…t pyceres (colmap#2985) Relevant discussion here: colmap#2978 No need for ``import ceres`` in ``custom_bundle_adjustment.py`` anymore (relevant discussion: colmap#2896) --------- Co-authored-by: Johannes Schönberger <jsch@demuc.de>