Add MaskedL2NN

[ahendriksen]

This PR adds the sparseL2NN functionality.

This enables faster computing pairwise distances by making use of sparsity in the problem: the computation of distances between point pairs can be skipped.

The sparsity between arrays of points X and Y is expressed as follows:

• X is split into rows (points)
• Y is split into contiguous groups of points (i.e. all points in a group are adjacent)
• A boolean adjacency matrix indicates for each row of X and each group in Y whether to compute the distance.

To speed up computation, the adjacency matrix is compressed into a bitfield.
To ensure competitive speeds, the caller must make sure that consecutive rows in X are adjacent to the same groups in Y (as much as possible) to enable efficient skipping in the kernel.

Some work is still TODO:

• Flesh out documentation
• Discuss / remove allocation of intermediate array
• Optimize for skinny matrices by using a different KernelPolicy.

[cjnolet]

@ahendriksen, it's getting very close to burndown for 22.12. Mind if we bump this forward to 23.02? Do you think you might have something working by then?

[ahendriksen]

Yes that is okay.

…

On Wed, Nov 9, 2022, at 9:00 PM, Corey J. Nolet wrote: @ahendriksen <https://github.com/ahendriksen> mind if we bump this forward to 23.02? Do you think you might have something working by then? — Reply to this email directly, view it on GitHub <#838 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA72YFTLXXO3A5U7QTFWVTDWHP7FBANCNFSM6AAAAAAQTBOGFU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[cjnolet]

Hey @ahendriksen, we are still really excited about this feature. I was looking through your changes a bit mostly to see where we are with it. I have a few review items but I've held off from submitting the review until you feel this is in a good place to do so.

To ensure competitive speeds, the caller must make sure that consecutive rows in X are adjacent to the same groups in Y (as much as possible) to enable efficient skipping in the kernel.

This won't be easy to do in an algorithm like HDBSCAN without modifying the user's input data, though I understand the importance of memory co-location like coalescing the reads. The pattern that this PR solves pops up quite a bit, though. Do you know the expected perf hit from not co-locating the data points w/ the groups? Is it pretty much all from coalescing?

[ahendriksen]

Hi @cjnolet,

Feel free to add your comments!

To ensure competitive speeds, the caller must make sure that consecutive rows in X are adjacent to the same groups in Y (as much as possible) to enable efficient skipping in the kernel.

This won't be easy to do in an algorithm like HDBSCAN without modifying the user's input data, though I understand the importance of memory co-location like coalescing the reads. The pattern that this PR solves pops up quite a bit, though. Do you know the expected perf hit from not co-locating the data points w/ the groups? Is it pretty much all from coalescing?

Coalescing is absolutely necessary.
I ran a representative benchmark where roughly 99% of distance calculations could be skipped. In the theoretical best case, sparseL2NN should have taken 1% of the time of fusedL2NN. In practice, the relative performance was

• ~98% without coalescing (sorting input points);
• ~10% with coalescing.

There will be a cut off point before which it does not make sense to sort the inputs. However, since the number of distance calculations scales quadratically with the number of input points, sorting will be worth it especially for medium to large data sizes.

As you suggest, the sorting may even be done in place and reverted after the sparseL2NN call (rather than allocating a separate buffer with sorted inputs).

[ajschmidt8]

Please use GitHub's Draft PR feature instead of WIP tags in the future. Draft PRs have the benefit of preventing notifications to codeowners until PRs are marked Ready for Review, which helps cut down on excessive notifications for PRs that are still being worked on. CI will still run on Draft PRs.

Some useful information about Draft PRs:

• https://github.blog/2019-02-14-introducing-draft-pull-requests/
• https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests#draft-pull-requests
• https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request

[tfeher]

Thanks Allard for addressing the issues! The PR looks good to me!

[cjnolet]

@ahendriksen , the PR looks great. I have one additional ask, though. Can you create Github issues for these remaining items just so it stays on our radar?

I did not get around to:

Exposing compress_to_bits in raft/utils.
Adding an overload the accepts a bitfield.

[cjnolet]

@ahendriksen one thing that I've noticed is that this PR and #837 both seem to be consistently timing out in CI while there are some other PRs that don't seem to be timing out. This makes me a little nervous about immediately merging these right before burndown. In case this behavior is isolated to this PR, I want to be careful not to cause issues for other PRs during this release.

We still have some time before code freeze to merge these, but it would help if you are able to verify locally that the build time (and ideally memory requirement during building) doesn't seem to be drastically affected by these changes. Maybe compare the build time for your changes against branch-23.02 just to see? It could be something super simple- like a pre-compiled template instantiation might no longer be getting used which could be causing things to get re-compiled more times from scratch.

[cjnolet]

Holding off on merging right away so we can investigate the CI timeouts.

[cjnolet]

@ahendriksen can you please update this PR to accept raft::device_resources everywhere instead of raft::handle_t?

[ahendriksen]

@ahendriksen can you please update this PR to accept raft::device_resources everywhere instead of raft::handle_t?

Done.

[ahendriksen]

@ahendriksen , the PR looks great. I have one additional ask, though. Can you create Github issues for these remaining items just so it stays on our radar?

I have created issues:

• [FEA] Have maskedL2NN accept bitfields directly #1196
• [FEA] Expose compress to bits in raft/utils #1195

[cjnolet]

/merge