Skip to content

Add option to pass initial model to robust solvers. #136

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 14, 2025
Merged

Add option to pass initial model to robust solvers. #136

merged 1 commit into from
Apr 14, 2025

Conversation

ghanning
Copy link
Contributor

@ghanning ghanning commented Apr 8, 2025 •
edited
Loading

See #134.

In C++ this is controlled by the new score_initial_model flag in RansacOptions.

When using the Python bindings the user should pass the initial model as an optional argument.

vlarsson
vlarsson reviewed Apr 8, 2025
View reviewed changes
vlarsson
vlarsson reviewed Apr 8, 2025
View reviewed changes
@ghanning ghanning force-pushed the initial-model branch 4 times, most recently from 26c4b44 to eba8315 Compare April 11, 2025 13:24
@ghanning ghanning marked this pull request as ready for review April 11, 2025 13:25
ghanning
ghanning commented Apr 11, 2025
View reviewed changes
PoseLib/robust.cc
@@ -307,6 +307,10 @@ RansacStats estimate_fundamental(const std::vector<Point2D> &x1, const std::vect
ransac_opt_scaled.max_epipolar_error /= scale;
BundleOptions bundle_opt_scaled = bundle_opt;
bundle_opt_scaled.loss_scale /= scale;
if (ransac_opt.score_initial_model) {
*F = T2.transpose().inverse() * (*F) * T1.inverse();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check that this normalization is correct (and for H below).

Are there other estimators that I missed for which the input model has to be normalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably estimate_shared_focal_relative_pose (the focal length)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think this should probably be normalized as well. Perhaps re-initializing the cameras similar to

image_pair->camera1 = Camera("SIMPLE_PINHOLE", std::vector<double>{image_pair->camera1.focal() / scale, 0.0, 0.0}, -1, -1);
image_pair->camera2 = Camera("SIMPLE_PINHOLE", std::vector<double>{image_pair->camera2.focal() / scale, 0.0, 0.0}, -1, -1);

or something similar.

@ghanning
Copy link
Contributor Author

Some results from posebench where I first ran the estimators as usual and the re-ran with the estimated model as the initial one:

                          AUC0   AUC1   AUC5   med_rot   med_pos   avg_rt   med_rt
PnP (poselib)            50.51  89.72  96.95   3.3E-01       0.1   14.7ms   13.3ms
PnP (poselib) (initial)  50.54  89.72  96.94   3.3E-01       0.1    7.3ms    6.8ms

                        AUC5   AUC10   AUC20   avg_rt   med_rt
E (poselib)            49.14   63.49   74.70   40.1ms   36.1ms
E (poselib) (initial)  49.15   63.50   74.71   27.8ms   24.9ms
F (poselib)            34.04   47.86   60.51   40.3ms   34.8ms
F (poselib) (initial)  34.09   47.91   60.54   19.9ms   16.7ms

                        AUC5   AUC10   AUC20   avg_rt   med_rt
H (poselib)             0.86    2.54    7.15  762.3us  545.2us
H (poselib) (initial)   0.86    2.58    7.21  654.5us  517.5us

The AUC values are very similar and the runtime goes down a bit when using the initial model. I did not change the min_iterations parameter (it was set to the default value 100). Note that not all estimators are included in the benchmark.

@vlarsson
Copy link
Collaborator

@ghanning this looks good to me. Are there any changes planned or is it ready to merge?

@ghanning
Copy link
Contributor Author

@vlarsson Ready to merge.

@pablospe pablospe merged commit a37d2bd into PoseLib:master Apr 14, 2025
1 check passed
@vlarsson vlarsson mentioned this pull request Jul 4, 2025
Closed
@vlarsson vlarsson mentioned this pull request Aug 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablospe pablospe pablospe approved these changes

@vlarsson vlarsson vlarsson approved these changes

@Parskatt Parskatt Awaiting requested review from Parskatt Parskatt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ghanning @vlarsson @pablospe