Add heterogeneous add_self_loop support #345
Conversation
|
Regarding the method |
src/GNNGraphs/transform.jl
Outdated
|
|
||
| n = get(g.num_nodes, src_t, 0) | ||
|
|
||
| # By avoiding using haskey, this only calls ht_keyindex once instead of twice |
There was a problem hiding this comment.
| # By avoiding using haskey, this only calls ht_keyindex once instead of twice |
These kinds of performance concerns for dictionary queries are irrelevant. The heavy operations are the copies and the concatenations. Therefore the concern here should be to make the code has readable as possible, which means using haskey in my opinion.
src/GNNGraphs/transform.jl
Outdated
| s = [s; nodes] | ||
| t = [t; nodes] | ||
| else | ||
| nodes = [1:n;] |
There was a problem hiding this comment.
This creates a cpu array even if the rest of graph lives on gpu. We should create a new array similar to one in the existing relations.
Update repo
…Bail/GraphNeuralNetworks.jl into add_self_loops-heterograph Pull from upstream
|
I believe this to now be initializing edge arrays with the correct type, but please check to confirm. I also extended the test cases marginally. Tests should pass on latest, but not on nightly, similarly to #356. |
|
nice, thanks! |
Here is the PR as discussed in #329.