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

Deterministic registration #3737

Merged
merged 6 commits into from
Jul 31, 2021

Conversation

gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Jul 14, 2021

This PR addresses #1263 for allowing deterministic registration:

  • Adds a seed optional parameter to RegistrationRANSACBasedOnCorrespondence and RegistrationRANSACBasedOnFeatureMatching
  • Adds a seed member to FastGlobalRegistrationOption
  • Replaces the UniformRandInt function with a callable class that accepts an optional seed.

This change is Reviewable

@update-docs
Copy link

update-docs bot commented Jul 14, 2021

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@ssheorey ssheorey requested review from theNded, yxlao and ssheorey July 16, 2021 18:46
Copy link
Contributor

@theNded theNded left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gsakkis, @ssheorey, and @yxlao)


cpp/open3d/pipelines/registration/FastGlobalRegistration.h, line 73 at r1 (raw file):

                                 double tuple_scale = 0.95,
                                 int maximum_tuple_count = 1000,
                                 unsigned int seed = std::random_device{}())

Also use utility::nullopt here for consistency?


cpp/tests/core/Tensor.cpp, line 1702 at r1 (raw file):

    int64_t max_size = *std::max_element(sizes.begin(), sizes.end());
    std::vector<int> vals(max_size);
    utility::UniformRandInt dist_gen(0, 3);

Potentially add a test in registration to verify that the results are deterministic?

@gsakkis gsakkis force-pushed the gsk/deterministic-registration branch from 10c0819 to 7d419fd Compare July 26, 2021 09:36
cpp/open3d/utility/Helper.h Outdated Show resolved Hide resolved
cpp/tests/core/Tensor.cpp Outdated Show resolved Hide resolved
cpp/open3d/pipelines/registration/FastGlobalRegistration.h Outdated Show resolved Hide resolved
@gsakkis gsakkis force-pushed the gsk/deterministic-registration branch from 7d419fd to 258d0d2 Compare July 27, 2021 09:48
@gsakkis
Copy link
Contributor Author

gsakkis commented Jul 27, 2021

Addressed all comments, thanks for the review.

Copy link
Contributor

@theNded theNded left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r2, 6 of 6 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gsakkis and @ssheorey)

@yxlao yxlao merged commit 92bfc40 into isl-org:master Jul 31, 2021
@gsakkis gsakkis deleted the gsk/deterministic-registration branch July 31, 2021 17:28
@dalnoguer
Copy link

Hello, it seems that the seed is not exposed in python. Would it be possible to do so?

(Pdb) o3d.pipelines.registration.FastGlobalRegistrationOption(maximum_correspondence_distance=distance_threshold, seed=0)
*** TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
    1. open3d.cuda.pybind.pipelines.registration.FastGlobalRegistrationOption(arg0: open3d.cuda.pybind.pipelines.registration.FastGlobalRegistrationOption)
    2. open3d.cuda.pybind.pipelines.registration.FastGlobalRegistrationOption(division_factor: float = 1.4, use_absolute_scale: bool = False, decrease_mu: bool = False, maximum_correspondence_distance: float = 0.025, iteration_number: int = 64, tuple_scale: float = 0.95, maximum_tuple_count: int = 1000, tuple_test: bool = True)

Invoked with: kwargs: maximum_correspondence_distance=0.005, seed=0

Thanks!

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