dmlc / dgl

Python package built to ease deep learning on graph, on top of existing DL frameworks.
http://dgl.ai
Apache License 2.0
13.53k stars 3.01k forks source link

[Feat request] G.all_edges support order='dst' #3337

Open felipemello1 opened 3 years ago

felipemello1 commented 3 years ago

🚀 Feature

When using G.all_edges, only 3 types of ordering are supported: None, 'eid', 'srcdst'. Request: Add the option order='dst', if it is easy to do so.

Motivation

I need to calculate:

uid, vid = G.all_edges(form='uv', order=None, etype=etype)
unique_vid = torch.unique(vid)

But torch.unique is 20% of the computation of my whole function, and torch.unique_consecutive is 10x faster. torch.unique_consecutive assumes that the vector is already sorted, so if G.all_edges could already provide sorted vid at minimal cost, the function would be 18% faster.

Additional context

I couldn't find the documentation of G.all_edges in the website for versions after 0.6.x.

Rhett-Ying commented 3 years ago

It's named as g.edges() now...

Rhett-Ying commented 3 years ago

could you try https://docs.dgl.ai/generated/dgl.reverse.html#dgl.reverse with order=srcdst

felipemello1 commented 3 years ago

could you try https://docs.dgl.ai/generated/dgl.reverse.html#dgl.reverse with order=srcdst

Hi @Rhett-Ying , thanks for getting back to me. I understand that I could use reverse, but I would have to pay attention to semantics issues 'user', 'plays', 'game' --> 'game', 'plays', 'user', and possibly speed issues.

I guess what I am thinking is that if DGL already has the edges sorted by 'dst', and getting hg.all_edges(order='dst) is just a matter on changing a couple of lines inside of the code, it could be worthwhile.

Rhett-Ying commented 3 years ago

How about uid, vid = hg.in_edges(hg.nodes('game'), etype='plays', form='uv') ?

felipemello1 commented 3 years ago

uid, vid = hg.in_edges(hg.nodes('game'), etype='plays', form='uv')

@Rhett-Ying Wouldn't this limit the search for only 'in_edges' instead of all edges, or am I interpreting G.all_edges wrong?

Rhett-Ying commented 3 years ago

for heterographs, a non-ambiguous etype name is required. for each non-ambiguous etype name, only one in/out ntype exists. so hg.in_edges should works same as hg.edges().