Self-loops management in KNNGraph and RadiusGraph

[FilippoOlivo]

Fixes #521

[GiovanniCanali]

Hi @FilippoOlivo!
I have noticed you are removing self loops when building the RadiusGraph. In order to be compliant with pyg, I suggest to handle this through a boolean attribute loop. If True, the graph will contain self loops.

See here for reference: https://pytorch-geometric.readthedocs.io/en/latest/generated/torch_geometric.nn.pool.radius_graph.html#torch_geometric.nn.pool.radius_graph

What do you think, @dario-coscia?

[dario-coscia]

Hi @FilippoOlivo! I have noticed you are removing self loops when building the RadiusGraph. In order to be compliant with pyg, I suggest to handle this through a boolean attribute loop. If True, the graph will contain self loops.

See here for reference: https://pytorch-geometric.readthedocs.io/en/latest/generated/torch_geometric.nn.pool.radius_graph.html#torch_geometric.nn.pool.radius_graph

What do you think, @dario-coscia?

yes maybe it is the best option!

[FilippoOlivo]

Since the codacy warning is "intentional" and unavoidable in our case, I suggest to manually suppress it. @ndem0 @dario-coscia what do you think?

[dario-coscia]

I don't like a lot the fact that loop is hard coded for both methods. I suggest to add the loop attribute in GraphBuilder __new__ method and use the PyG function remove_self_loops once the edge_index is created. This way you just need to touch GraphBuilder

[FilippoOlivo]

I don't like a lot the fact that loop is hard coded for both methods. I suggest to add the loop attribute in GraphBuilder __new__ method and use the PyG function remove_self_loops once the edge_index is created. This way you just need to touch GraphBuilder

A possible inconsistency arises with KNNGraph. K should include or not self loops?

[dario-coscia]

I don't like a lot the fact that loop is hard coded for both methods. I suggest to add the loop attribute in GraphBuilder __new__ method and use the PyG function remove_self_loops once the edge_index is created. This way you just need to touch GraphBuilder

A possible inconsistency arises with KNNGraph. K should include or not self loops?

Why inconsistency? I would say that K is the number of points close to a reference point (and including the reference point), then if loop is False this is K-1. This way the behaviour should be similar to radius graph right?

[dario-coscia]

Everything looks good to me, I merge in dev.