Temporal sample disable undirected

[RexYing]

For temporal sampling, the undirected neighbor sampling does not make sense, since it may violate the time constraint when constructing subgraph from the sampled nodes.

Hence we disable directed=False case for temporal sampling. For direct=True case, we also skip the check if the node already existed, so that the same node (potentially with different time constraints due to different input nodes in the minibatch) can have multiple copies in the minibatch.

[codecov-commenter]

Codecov Report

Merging #247 (e98496f) into master (7dbc51c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #247   +/-   ##
=======================================
  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]

Hi @RexYing, I think the implementation is correct and looks good. Thank you!

If you spot any index_select errors during forward propagation, it is best to check that

for edge_type in data.edge_types:
    src, _, dst = edge_type    
    assert data[edge_type].edge_index[0].max() < data[src].num_nodes
    assert data[edge_type].edge_index[1].max() < data[dst].num_nodes

holds. Can you confirm?

[zechengz]

The rows.push_back(src_samples.size() - 1); will resolve the out of index error.

[rusty1s]

@zechengz Good catch, you are right :)