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 uniform and random down sample methods in Tensor PointCloud #5202

Merged
merged 9 commits into from
Jun 28, 2022

Conversation

yuecideng
Copy link
Collaborator

@yuecideng yuecideng commented Jun 15, 2022

Update
Use tensor slicing stead of indexing for uniform sampling, which makes it more faster.

This change is Reviewable

@update-docs
Copy link

update-docs bot commented Jun 15, 2022

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

@yuecideng
Copy link
Collaborator Author

yuecideng commented Jun 16, 2022

Benchmark for UniformDownSample.

image

return pcd_down;
}

PointCloud PointCloud::RandomDownSample(double sampling_ratio) const {
Copy link
Member

Choose a reason for hiding this comment

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

  1. For reproducible experiments, we need the ability to set the random seed for all RNG calls by Open3D. One way to achieve this is with a singleton RNG, with some care for concurrent access.
  2. CUDA has an RNG that will likely be faster for CUDA tensors.

@reyanshsolis
Copy link
Collaborator

reyanshsolis commented Jun 17, 2022

I appreciate the effort, but I was wondering if there is any use case for this functionality?

Here uniform is misleading, as the point cloud is unorganized data, so removing every k-th point, will not downsample the point cloud uniformly.

Do we have a reference implementation/inspiration of such functionality in PCL or other 3D data processing packages?

@reyanshsolis
Copy link
Collaborator

Voxel down-sampling is actually a uniform down-sample in a way.
However, it lacks the option of averaging or sampling, i.e. control if the new point attributes are the mean of the points and their attributes within a voxel, or it's one of the existing points (sampled).

I guess users might appreciate an improved voxel-down-sample functionality over other filtering options.

@yuecideng
Copy link
Collaborator Author

The uniform here means taking a subset of point cloud with the same interval w.r.t original order. The use case is to provide a simple interface for user to fast down sample a very large pcd (eg. 10M points, particularly for industrial scenario, this can be commomly seen). Sometimes it is no worth to use VoxelDownSample when consider time comsuming. Beside, we also provide this function in legacy version.

However, it lacks the option of averaging or sampling, i.e. control if the new point attributes are the mean of the points and their attributes within a voxel, or it's one of the existing points (sampled).

I agree with this because currenly the voxel down sample method in tensor based pcd only return coordinates of voxels, leading to the inconsistent with legacy version and other library's behaviour.

/// \param voxel_size Voxel size. A positive number.
PointCloud VoxelDownSample(double voxel_size,
const core::HashBackendType &backend =
core::HashBackendType::Default) const;

/// \brief Downsamples a point cloud uniformly
Copy link
Collaborator

Choose a reason for hiding this comment

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

please elaborate on the description such as:
Downsamples a point cloud, by selecting every kth index point and its attributes.

Similarly for RandomDownSample

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -198,6 +198,18 @@ The attributes of the point cloud have different levels::
},
"Downsamples a point cloud with a specified voxel size.",
"voxel_size"_a);
pointcloud.def(
"uniform_down_sample",
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may use the following:
Instead of

pointcloud.def(
            "uniform_down_sample",
            [](const PointCloud& pointcloud, const size_t every_k_points) {
                return pointcloud.UniformDownSample(every_k_points);
            },
            "Downsamples a point cloud uniformly.", "every_k_points"_a);

you may try:

pointcloud.def(
          "uniform_down_sample", &UniformDownSample,
          "Downsamples a point cloud uniformly.", "every_k_points"_a);

similarly to other functions.

Also, test the same if it is working, you may find similar use in pybind folder for reference.

Copy link
Collaborator Author

@yuecideng yuecideng Jun 20, 2022

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@reyanshsolis reyanshsolis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @yuecideng and @yxlao)


cpp/tests/t/geometry/PointCloud.cpp line 770 at r2 (raw file):

    core::Device device = GetParam();

    // Value test

Add a .


cpp/tests/t/geometry/PointCloud.cpp line 789 at r2 (raw file):

    core::Device device = GetParam();

    // Value test

same

Copy link
Collaborator Author

@yuecideng yuecideng left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @reyanshsolis, @yuecideng, and @yxlao)


cpp/tests/t/geometry/PointCloud.cpp line 770 at r2 (raw file):

Previously, reyanshsolis (Rishabh Singh) wrote…

Add a .

Done


cpp/tests/t/geometry/PointCloud.cpp line 789 at r2 (raw file):

Previously, reyanshsolis (Rishabh Singh) wrote…

same

Done

@heethesh
Copy link
Contributor

#4849

@yuecideng
Copy link
Collaborator Author

Update with using open3d random singleton. CUDA implementation will be added in the future PR.

Copy link
Collaborator

@yxlao yxlao 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 2 files at r2, 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 4 of 5 files reviewed, 8 unresolved discussions (waiting on @reyanshsolis, @ssheorey, @yuecideng, and @yxlao)


cpp/benchmarks/t/geometry/PointCloud.cpp line 123 at r5 (raw file):

    pcd = pcd.To(device);

    // Warp up

typo: warm up.


cpp/pybind/t/geometry/pointcloud.cpp line 204 at r5 (raw file):

                   "Downsamples a point cloud by selecting every kth index "
                   "point and its attributes.",
                   "every_k_points"_a);

Same as below, call docstring::ClassMethodDocInject


cpp/pybind/t/geometry/pointcloud.cpp line 208 at r5 (raw file):

                   "Downsample a pointcloud by selecting random index point "
                   "and its attributes.",
                   "sampling_ratio"_a);

We need to call docstring::ClassMethodDocInject to enable the docs. In our case, we need to document sampling_ratio by passing in a dictionary.

Code quote:

docstring::ClassMethodDocInject

Copy link
Collaborator Author

@yuecideng yuecideng left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 5 files reviewed, 8 unresolved discussions (waiting on @reyanshsolis, @ssheorey, @yuecideng, and @yxlao)


cpp/pybind/t/geometry/pointcloud.cpp line 208 at r5 (raw file):

Previously, yxlao (Yixing Lao) wrote…

We need to call docstring::ClassMethodDocInject to enable the docs. In our case, we need to document sampling_ratio by passing in a dictionary.

Should we also add docstring::ClassMethodDocInject to the recently added functions, like SelectByMask, SelectByIndex, RemoveRadiusOutliers? I think it is better to add them all.

Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 5 files reviewed, 9 unresolved discussions (waiting on @reyanshsolis, @ssheorey, @yuecideng, and @yxlao)


cpp/open3d/t/geometry/PointCloud.cpp line 359 at r6 (raw file):

                false, false);
    } else {
        utility::LogError("RandomDownSample is not supported on GPU.");

LGTM, after fixing this. We can copy the indices from CPU to CUDA for now. This is required to pass the CUDA CI test.

Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 5 files reviewed, 9 unresolved discussions (waiting on @reyanshsolis, @ssheorey, @yuecideng, and @yxlao)

@yxlao yxlao merged commit 3bf5bf0 into master Jun 28, 2022
@yxlao yxlao deleted the yueci/tpcd-sampling-method branch June 28, 2022 06:02
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.

5 participants