Add tests for neighbor sampling

[Padarn]

Addresses #208

This fixes the case where repeated nodes are provided to the neighbour_sampling functions. Previously, the "local graph" would essentially create a node for each of these repeats, now repeats are ignored.

Additionally adding tests for this op.

Remaining to do

• Add heterogeneous graph tests

[Padarn]

I didn't do anything about the idea of creating separate graphs for each input node here: Do you think this is best added as an option in torch_sparse or in torch_geometric

Also I added a question on the issue this references. I thought it would be good to add some extra documentation to this function as its a bit hard to tell what it does, but I wasn't sure how to do this in torch_sparse cpp extensions so that it fits with the rendered documentation.

What do you think?

[codecov-commenter]

Codecov Report

Merging #210 (3717cac) into master (b38d0b5) will not change coverage.
The diff coverage is n/a.

❗ Current head 3717cac differs from pull request most recent head cf6ad69. Consider uploading reports for the commit cf6ad69 to get more accurate results

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   72.32%   72.32%           
=======================================
  Files          28       28           
  Lines        1120     1120           
=======================================
  Hits          810      810           
  Misses        310      310           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[rusty1s]

Thank you. I am personally a bit worried about the implications of this PR. In the end, this means that we no longer have a 1-to-1 mapping between node indices in input_nodes and the resulting samples vector.

It would be great to make this functionality at least optional.

[Padarn]

Yeah, understand ... It seems a bit dangerous to assume that this is how it is being used, even if I think the behavior is unclear currently. I don't think its nice to add yet another input argument to the registered pytorch op, so I would propose either this is optional in the `pytorch_geometric` change (i.e do deduplication there), or I register another op that works this way (the functions in `neighbour_sample_cpu` could be reused. Personally, although it may be less efficient, I'd be in favor of just doing this all in `pytorch_geometric`. Reasoning: - It's perhaps slightly more expensive to dedupe separately, but maybe not too bad and given its new functionality there will be no impact on existing users. - It'd be better to keep this library smaller/cleaner until it is clear this is functionality that is needed in more than one place. Thoughts? Happy to abandon this if we're aligned.

[Padarn]

Actually while working on the pytorch_geometric issue I noticed another problem with trying to implement link-based sampling using this function: There is no guarantee that given nodes X and Y, the edge X->Y will be sampled (unless directed is set to False)

I'm thinking the best way to get somewhere quickly would be to implement the link sampling in torch/torch_geometric code (similar to the old one) and then see if we want to move some of the functionality into torch_sparse.

[rusty1s]

Yeah, let's do this step directly in PyG (which can be solved by a simple torch.unique call I think). With unique(return_inverse=True), you should be able to get the inverse mapping as well.

IMO, the edge X->Y do not need to be sampled. For link-level predictions, we only care of about sampling neighborhoods around X and Y to obtain their embeddings, and then try to predict the link X->Y based on it. This holds true independent of whether X and Y are originally connected or not.

[Padarn]

You're right. It seemed weird the edge not being sampled, but there is indeed no real reason to need this...

Thanks for the discussion and clearing things up for me!

[rusty1s]

@Padarn I re-opened the PR to include the test suite of neighbor_sample. Thanks a lot :)

[Padarn]

Welcome!