Skip to content

feat: Added bindings for ProductManifold#88

Merged
B1ueber2y merged 2 commits intocvg:mainfrom
TannerGilbert:feat/product_manifold_bindings
Apr 8, 2026
Merged

feat: Added bindings for ProductManifold#88
B1ueber2y merged 2 commits intocvg:mainfrom
TannerGilbert:feat/product_manifold_bindings

Conversation

@TannerGilbert
Copy link
Copy Markdown
Contributor

@TannerGilbert TannerGilbert commented Apr 7, 2026

Adds bindings for ProductManifold, needed by the COLMAP IMU demo (colmap/colmap#2625) and more generally useful since colmap::Rigid3d became a parameter block in 4.0.0.

Signed-off-by: Gilbert Tanner <gilberttanner.work@gmail.com>
Copy link
Copy Markdown
Member

@B1ueber2y B1ueber2y left a comment

Choose a reason for hiding this comment

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

Thank you!! I m wondering if it is really necessary to rewrite everything instead of directly binding the ceres::ProductManifold out. I guess you tried it but did not manage to get it working. Can you clarify what was the blocker?

On the other hand, re your PR description, this feature is highly useful not only for IMU but also in general as colmap::Rigid3d uses a single parameter block since the 4.0.0 release.

Comment thread examples/test_product_manifold.py Outdated
Comment thread examples/test_product_manifold.py Outdated
Comment thread examples/test_product_manifold.py
@TannerGilbert
Copy link
Copy Markdown
Contributor Author

Thank you!! I m wondering if it is really necessary to rewrite everything instead of directly binding the ceres::ProductManifold out. I guess you tried it but did not manage to get it working. Can you clarify what was the blocker?

On the other hand, re your PR description, this feature is highly useful not only for IMU but also in general as colmap::Rigid3d uses a single parameter block since the 4.0.0 release.

Thanks for the review. Honestly, I'm not super familiar with pybind11, and therefore don't know how to best bind variadic class templates.

template <typename Manifold0, typename Manifold1, typename... ManifoldN>
class ProductManifold final : public Manifold { ... };  

My first try was to just bind ceres::ProductManifold directly, but I couldn't figure out how to make it work and therefore switched to rewriting it. If there is a way to handle class templates like the one above without this I'd be happy to make the changes after having a look.

@TannerGilbert
Copy link
Copy Markdown
Contributor Author

Relevant docs: https://pybind11.readthedocs.io/en/stable/advanced/classes.html#binding-classes-with-template-parameters

C++ templates may only be instantiated at compile time, so pybind11 can only wrap instantiated templated classes. You cannot wrap a non-instantiated template

You must explicitly specify each template/type combination that you want to wrap separately.

@B1ueber2y
Copy link
Copy Markdown
Member

I see. Sounds good. Let s go with the current way then. Reminder not to forget to format.

@TannerGilbert
Copy link
Copy Markdown
Contributor Author

Tried to implement the changes and also applied formating with clang-format and ruff respectively.

Copy link
Copy Markdown
Member

@B1ueber2y B1ueber2y left a comment

Choose a reason for hiding this comment

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

Thank you. The formatting CI still failed though. Please make sure that your clang-format version matches the one used in this CI (which might be different from what is used in colmap).

Signed-off-by: Gilbert Tanner <gilberttanner.work@gmail.com>
@TannerGilbert TannerGilbert force-pushed the feat/product_manifold_bindings branch from b364388 to af106cc Compare April 7, 2026 22:30
@TannerGilbert
Copy link
Copy Markdown
Contributor Author

Just checked again and now used the provided .clang-format file.

@B1ueber2y B1ueber2y merged commit ae2d85d into cvg:main Apr 8, 2026
5 checks passed
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.

2 participants