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

DBSCAN clustering algorithm #1038

Merged
merged 2 commits into from
Jun 26, 2019
Merged

DBSCAN clustering algorithm #1038

merged 2 commits into from
Jun 26, 2019

Conversation

griegler
Copy link
Contributor

@griegler griegler commented Jun 24, 2019

Implementation of the DBSCAN clustering algorithm for PointCloud. Implementation is not threaded for now, but could be a simple addition.


This change is Reviewable

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.

Great work! Added some small comments.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @griegler)


src/Open3D/Geometry/PointCloudCluster.cpp, line 52 at r1 (raw file):

        std::vector<int> nbs;
        std::vector<double> dists2;
        kdtree.SearchRadius(points_[idx], eps2, nbs, dists2);

from https://github.com/intel-isl/Open3D/blob/e151b0bc0e95e7c3605fd7627335b648b2f555a3/src/Open3D/Geometry/KDTreeFlann.cpp#L152-L153, looks like the esp is squared internally by SearchRadius, so we shall probably use esp instead of esp2?


src/Open3D/Geometry/PointCloudCluster.cpp, line 60 at r1 (raw file):

        }

        std::unordered_set<int> nbs_next(nbs.begin(), nbs.end());

maybe it's cleaner to use std::deque<int>, orstd::stack<int> (if we conver to DFS) ? seems like we're not using the "set" property here.


src/Open3D/Geometry/PointCloudCluster.cpp, line 78 at r1 (raw file):

            labels[nb] = cluster_label;

            kdtree.SearchRadius(points_[nb], eps2, nbs, dists2);

same, esp2 -> esp?

Copy link
Contributor Author

@griegler griegler 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: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @yxlao)


src/Open3D/Geometry/PointCloudCluster.cpp, line 52 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

from https://github.com/intel-isl/Open3D/blob/e151b0bc0e95e7c3605fd7627335b648b2f555a3/src/Open3D/Geometry/KDTreeFlann.cpp#L152-L153, looks like the esp is squared internally by SearchRadius, so we shall probably use esp instead of esp2?

thanks, changed it to eps.


src/Open3D/Geometry/PointCloudCluster.cpp, line 60 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

maybe it's cleaner to use std::deque<int>, orstd::stack<int> (if we conver to DFS) ? seems like we're not using the "set" property here.

the set property is used in the insert step. In deque, or stack it would possibly insert some indices multiple times.


src/Open3D/Geometry/PointCloudCluster.cpp, line 78 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

same, esp2 -> esp?

done.

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: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @griegler and @yxlao)


src/Open3D/Geometry/PointCloudCluster.cpp, line 60 at r1 (raw file):
From here, are we already checking that nbs_visited.count(qnb) == 0 before inserting?

                    if (nbs_visited.count(qnb) == 0) {
                        nbs_next.insert(qnb);
                    }

Copy link
Contributor Author

@griegler griegler 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: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @yxlao)


src/Open3D/Geometry/PointCloudCluster.cpp, line 60 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

From here, are we already checking that nbs_visited.count(qnb) == 0 before inserting?

                    if (nbs_visited.count(qnb) == 0) {
                        nbs_next.insert(qnb);
                    }

but nbs_visited and nbs_next are different. A point can be inserted multiple times into nbs_next before it is actually visited (and inserted into nbs_visited).

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: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @griegler and @yxlao)


src/Open3D/Geometry/PointCloudCluster.cpp, line 60 at r1 (raw file):

Previously, griegler (Gernot) wrote…

but nbs_visited and nbs_next are different. A point can be inserted multiple times into nbs_next before it is actually visited (and inserted into nbs_visited).

i see, this could be more efficient, thanks for the explanation

@yxlao yxlao merged commit 9111fc8 into isl-org:master Jun 26, 2019
@yxlao yxlao deleted the dbscan branch June 26, 2019 09:42
@griegler griegler mentioned this pull request Jun 26, 2019
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.

None yet

2 participants