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

Support for precomputed distance matrix in DBSCAN #3585

Merged
merged 10 commits into from
Mar 30, 2021

Conversation

Nyrio
Copy link
Contributor

@Nyrio Nyrio commented Mar 4, 2021

Closes #3302

Notes about performance

If we don't count the cost of pre-computing the distance matrix (which is done by the user), the single-GPU code runs slightly faster when the distance matrix is pre-computed. (note: this is 2d, greater speedups expected for larger dimensions!)

dbscan_precomputed_sg
dbscan_precomputed_timeline

As I have stated in a comment in the code, it works with two kernels: one that uses a coalesced reduction to compute the vertex degrees from the distance matrix, and one that uses a 2D copy fused with an unary operation to get the boolean neighborhood matrix.

Note: the performance of this step could be better if adj was a row-major B*N matrix instead of column-major. We could fuse everything into one efficient kernel. It is something to keep in mind when we re-write csr_adj_graph_batched.

Notes about MNMG

Cf #3615

@Nyrio Nyrio requested review from a team as code owners March 4, 2021 20:18
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Mar 4, 2021
@Nyrio Nyrio added 2 - In Progress Currenty a work in progress feature request New feature or request labels Mar 4, 2021
…ore robust to index overflows with the distance matrix, test multi-batch cases
@Nyrio Nyrio changed the title [WIP] Support for precomputed distance matrix in DBSCAN Support for precomputed distance matrix in DBSCAN Mar 5, 2021
@Nyrio Nyrio added 3 - Ready for Review Ready for review by team non-breaking Non-breaking change and removed 2 - In Progress Currenty a work in progress labels Mar 5, 2021
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @Nyrio for the PR! It looks good in general, I have just marked a few smaller issues.

Please file an issue about the MNMG case with distributed data, to promote discussion on its priority.

python/cuml/test/test_dbscan.py Outdated Show resolved Hide resolved
python/cuml/test/dask/test_dbscan.py Outdated Show resolved Hide resolved
python/cuml/cluster/dbscan.pyx Outdated Show resolved Hide resolved
cpp/src/dbscan/vertexdeg/precomputed.cuh Show resolved Hide resolved
cpp/src/dbscan/vertexdeg/precomputed.cuh Outdated Show resolved Hide resolved
cpp/src/dbscan/vertexdeg/precomputed.cuh Outdated Show resolved Hide resolved
cpp/src/dbscan/vertexdeg/precomputed.cuh Outdated Show resolved Hide resolved
cpp/src/dbscan/vertexdeg/precomputed.cuh Outdated Show resolved Hide resolved
cpp/test/sg/dbscan_test.cu Outdated Show resolved Hide resolved
python/cuml/test/test_dbscan.py Outdated Show resolved Hide resolved
@Nyrio Nyrio added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Mar 11, 2021
@Nyrio Nyrio requested a review from tfeher March 12, 2021 16:50
@Nyrio Nyrio added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Mar 12, 2021
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @Nyrio for the update. Changes look good in general, there are only some minor things left:

  • Fix the failing test_base_children_get_param_names unit test by adding 'metric' to get_param_names() in dbscan.pyx.
  • The PR description will be added as a commit message of the merge commit. I think the TODO section can be removed from the description.
  • Consider linking the original feature request with the appropriate keyword.
  • Please file an issue about "distance matrix that is scattered across the nodes".

@Nyrio Nyrio requested a review from tfeher March 15, 2021 19:01
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks Louis for the update! The PR looks good to me.

@Nyrio Nyrio added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Mar 16, 2021
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

For official-sake, approving this PR based on the reviews done by @tfeher (he's not yet a cpp-codeowner). @dantegd or @JohnZed can we get python-side approval from one of you?

Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review, long week of reviews. Just had one question and one very minor suggestion to improve the python docstring, then it looks good in the Python side as well.

cpp/include/cuml/cluster/dbscan.hpp Outdated Show resolved Hide resolved
python/cuml/cluster/dbscan.pyx Outdated Show resolved Hide resolved
@JohnZed JohnZed added 4 - Waiting on Author Waiting for author to respond to review 0 - Blocked Cannot progress due to external reasons and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Mar 24, 2021
@JohnZed
Copy link
Contributor

JohnZed commented Mar 24, 2021

Blocked waiting on RAFT change

rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Mar 24, 2021
Suggested by @dantegd in: rapidsai/cuml#3585 (comment)

Authors:
  - Louis Sugy (@Nyrio)

Approvers:
  - Thejaswi. N. S (@teju85)

URL: #177
@Nyrio Nyrio requested a review from a team as a code owner March 24, 2021 19:11
@github-actions github-actions bot added the CMake label Mar 24, 2021
@Nyrio
Copy link
Contributor Author

Nyrio commented Mar 24, 2021

@dantegd @teju85 I made the change to use raft::distance::DistanceType but I'm having a very weird issue in dbscan.pyx that I don't understand. If I use DistanceType.Precomputed, I get a compiler error:

UnicodeEncodeError: 'latin-1' codec can't encode character '\u0308' in position 15936: ordinal not in range(256)

If I comment line 259, it compiles. The problem doesn't seem to be a special character, I've tried rewriting this line from scratch, also it compiles when commented. I don't see why the enum value would cause that either, the value is 100, which is under 256.

I'm quite confused. Does someone know what's happening here?

@dantegd
Copy link
Member

dantegd commented Mar 25, 2021

@Nyrio that's quite an odd issue, I would suggest merging branch-0.19 into the branch of the PR (to solve the copyright issues) and then we can see if the unicode error persists.

# Conflicts:
#	cpp/cmake/Dependencies.cmake
@Nyrio
Copy link
Contributor Author

Nyrio commented Mar 26, 2021

I've merged branch-0.19 but it unleashed a dependency nightmare. The latest changes seem to require Faiss 1.7.0 and the latest Rapids Docker images released a few hours ago come with Faiss 1.6.3. Forcing the update of the libfaiss package creates linking errors with other libraries...

Edit: it works with 11.2. The dependency issue was with 11.0

@Nyrio
Copy link
Contributor Author

Nyrio commented Mar 26, 2021

Update: I managed to solve my dependency issues with 11.2 but the problem of the odd codec error persists.

I've also tried using contiguous values in the enum, which didn't seem to work.

@tfeher
Copy link
Contributor

tfeher commented Mar 26, 2021

I confirm the problem with the enum. Python install fails as long as we have the Precomputed name here.

A workaround is to rename the enum to Precalculated. With that it compiles for me, but that requires another RAFT PR.

@Nyrio
Copy link
Contributor Author

Nyrio commented Mar 26, 2021

It turns out it was an encoding bug, with an invisible character that didn't appear in the editor. Fixed.

@Nyrio Nyrio added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 0 - Blocked Cannot progress due to external reasons 4 - Waiting on Author Waiting for author to respond to review labels Mar 26, 2021
@codecov-io
Copy link

Codecov Report

Merging #3585 (99797c5) into branch-0.19 (fd9ec89) will decrease coverage by 35.36%.
The diff coverage is 57.07%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-0.19    #3585       +/-   ##
================================================
- Coverage        80.70%   45.34%   -35.37%     
================================================
  Files              227      224        -3     
  Lines            17615    17189      -426     
================================================
- Hits             14217     7794     -6423     
- Misses            3398     9395     +5997     
Flag Coverage Δ
dask 45.34% <57.07%> (+0.35%) ⬆️
non-dask ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/cuml/cluster/kmeans.pyx 58.29% <ø> (-33.67%) ⬇️
python/cuml/common/memory_utils.py 65.85% <0.00%> (-13.27%) ⬇️
python/cuml/fil/fil.pyx 63.77% <ø> (-28.07%) ⬇️
python/cuml/model_selection/_split.py 5.58% <0.00%> (-84.78%) ⬇️
python/cuml/neighbors/__init__.py 100.00% <ø> (ø)
python/cuml/neighbors/ann.pyx 8.04% <0.00%> (-53.59%) ⬇️
python/cuml/neighbors/nearest_neighbors.pyx 41.47% <0.00%> (-51.18%) ⬇️
python/cuml/pipeline/__init__.py 0.00% <0.00%> (ø)
python/cuml/preprocessing/encoders.py 88.04% <ø> (-7.04%) ⬇️
python/cuml/solvers/qn.pyx 17.31% <7.14%> (-80.32%) ⬇️
... and 150 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0883026...99797c5. Read the comment docs.

@dantegd
Copy link
Member

dantegd commented Mar 30, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4f4ae58 into rapidsai:branch-0.19 Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond CMake Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The support for "precomputed" in DBSCAN [FEA]
6 participants