[FEA] Add multi-groups version of the DBSCAN

[georgeliu95]

This draft PR is for the discussion on implementation details.

[rapids-bot]

Pull requests from external contributors require approval from a rapidsai organization member with write or admin permissions before CI can begin.

[cjnolet]

Hi @georgeliu95, thanks for contributing this feature. Can you please provide benchmarks to demonstrate the performance of this design compared to the existing dbscan implementation?

There's a lot of code in this PR taken and modified from the building blocks in RAFT and I'd like to keep the algorithms in cuml based on the reusable building blocks that have been centralized in RAFT. I think after benchmarks, a next step would be consolidating these back into RAFT. I'm also fine exposing these as multi-group building blocks if it makes sense.

Another note- we should prefer using raft::linalg::map_offset over thrust::for_each. We centralize these operations behind RAFT APIs so we have more control over future changes and optimizations.

[georgeliu95]

Thank you very much for your help and useful advice! @cjnolet
I just add the benchmark for multi-group DBSCAN and using raft::linalg::map / raft::linalg::map_offset instead of thrust::for_each as you suggested.
And if directly run this benchmark with cosine metric, there will be some difference compared to the baseline due to #5360.

[tfeher]

Thanks @georgeliu95 for this work! It is great to have an efficient implementation for performing a batch of smaller independent DBSCAN clustering!

I see that you arranged the new implementation to have a similar code structure as the existing DBSCAN code in cuML, thank you for this effort! I still think that we can sacrifice some of this similarity to simplify the code, and I have left a few comments along this line.

The main issue, as @cjnolet has already mentioned, is consolidating the computational primitives in raft (epsilon neighborhood, coalesced reduction, adj_to_csr). It is clear that we need a modified version of these primitives for the multi group case, but there is a large overlap between the existing single group version and the proposed multi-group variants. The goal is to reduce code duplication and improve maintainability of the code. These changes could be done in separate smaller PRs for raft.

Apart from these issues, the implementation looks good overall, and I am excited to have this feature added to cuML!

[tfeher]

Do we need extra workspace arg, as opposed to using a pooling allocator (like below), and keeping allocations inside? I am not opposed to the current API, just curious if we could simplify it without loosing perf.

rmm::mr::pool_memory_resource<rmm::mr::device_memory_resource> mr(rmm::mr::get_current_device_resource(), 1<<30);
rmm::mr::set_current_device_resource(&mr);
ML::Dbscan::fit(...)

[tfeher]

Please add a docstring.

[tfeher]

It is recommended to manage the lifetime of such allocation with a vector object, e.g.:

Suggested change

	h_adj_group_offset = new std::size_t[this->n_groups];
	h_adj_group_offset = raft::make_host_vector<size_t>(this->n_groups);

[tfeher]

Ideally the changes here should be added to raft to minimize code duplication.

[tfeher]

We should refactoring this into a gbench, and adding it to https://github.com/rapidsai/cuml/blob/branch-23.06/cpp/bench/sg/dbscan.cu

[tfeher]

Thanks @georgeliu95 for this work! It is great to have an efficient implementation for performing a batch of smaller independent DBSCAN clustering!

I see that you arranged the new implementation to have a similar code structure as the existing DBSCAN code in cuML, thank you for this effort! I still think that we can sacrifice some of this similarity to simplify the code, and I have left a few comments along this line.

The main issue, as @cjnolet has already mentioned, is consolidating the computational primitives in raft (epsilon neighborhood, coalesced reduction, adj_to_csr). It is clear that we need a modified version of these primitives for the multi group case, but there is a large overlap between the existing single group version and the proposed multi-group variants. The goal is to reduce code duplication and improve maintainability of the code. These changes could be done in separate smaller PRs for raft.

Apart from these issues, the implementation looks good overall, and I am excited to have this feature added to cuML!

[dantegd]

/ok to test