Skip to content

Uniform Multi-Hop Sampler - #63

Merged
Jacob-Chmura merged 16 commits into
mainfrom
dev/uniform_multi-hop
Jul 2, 2025
Merged

Uniform Multi-Hop Sampler#63
Jacob-Chmura merged 16 commits into
mainfrom
dev/uniform_multi-hop

Conversation

@Jacob-Chmura

Copy link
Copy Markdown
Member

Purpose

The purpose of this PR is to add multi-hop support to the COO based uniform neighbor sampler.

Performance

TODO: Need to validate correctness once #58 is in before writing extensive tests.

Key Changes

  • Storage engine only answers 1-hop queries now (the multi-hop aggregation is left to the hook (
    class NeighborSamplerHook:
    ). Note that the driver code is exactly the same between uniform sampler and recency sampler. The only difference is how we manage state and query the nbr ids. This is a good sign, and we could probably eventually write a base class hook (BaseNeighborSamplerHook) which manages the joint state and keeps the API fixed for all neighbour sampler implementations
  • Cleaned up the storage 1-hop nbr query design (and changed to API in the base class)

Relevant Prs

Close #12

Out of scope

  • Non-trivial performance optimizations

@Jacob-Chmura Jacob-Chmura self-assigned this Jun 25, 2025
Comment thread tgm/hooks.py Outdated
nbr_nids, nbr_times, nbr_feats, nbr_mask = dg._storage.get_nbrs(
seed_nodes,
num_nbrs=num_nbrs,
slice=DGSliceTracker(end_time=dg._slice.end_time),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, might have to be dg._slice.end_idx since we don't want it to look forward to the next batch (even if tiemstamps match)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand, to discuss

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end_time=dg._slice.start_time

@shenyangHuang shenyangHuang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to discuss at meeting

Comment thread examples/linkproppred/tgn.py Outdated
time_feat = self.time_encoder(torch.zeros(len(batch.nids[hop]), device=device))
nbr_time_feat = self.time_encoder(
batch.nbr_times[hop] - batch.time.unsqueeze(dim=1).repeat(3, 1)
batch.time.unsqueeze(dim=1).repeat(3, 1) - batch.nbr_times[hop]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is reversed?

# This is a loop over the entire graph up to (not including) the current batch end time
# which results in quadratic cost for a single epoch. Consider raises a warning to let the
# user know that this is not the right backend for this.
nbrs: Dict[int, List[Tuple[int, int]]] = {node: [] for node in seed_nodes_set}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the main cost part

nbrs[d].append((i, s))

B = len(seed_nodes)
nbr_nids = torch.full((B, num_nbrs), -1, dtype=torch.long, device=device)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worst case these are O(n), where n is the number of nodes in the graph.

nbr_nids_.append(self._data.edge_index[event_idx, edge_idx].item())
nbr_times_.append(self._data.timestamps[event_idx])
# Subsample if we have more neighbours than was queried
if num_nbrs != -1 and len(node_nbrs) > num_nbrs:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is also some online way of keeping neighbors right? some probabilistic approach that you mentioned? online way for unbiased sampling I mean. potentially a future enhancement

Comment thread tgm/hooks.py Outdated
nbr_nids, nbr_times, nbr_feats, nbr_mask = dg._storage.get_nbrs(
seed_nodes,
num_nbrs=num_nbrs,
slice=DGSliceTracker(end_time=dg._slice.end_time),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand, to discuss

Comment thread tgm/hooks.py Outdated
prev_hop_seed = batch.nids[-1] # type: ignore
prev_hop_nbrs = batch.nbr_nids[-1][batch.nbr_mask[-1].bool()] # type: ignore

# TODO: What is the expected behaviour when prev hop nbrs is empty?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if previous hop is empty, then the next hop is also empty

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for most methods, they care about the sampled neighborhood for all nodes in the batch at the end. If some nodes are disconnected, i.e. no neighbor, it will just message pass to itself only.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to discuss

@codecov

codecov Bot commented Jul 2, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 94.73684% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
tgm/hooks.py 87.50% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Jacob-Chmura
Jacob-Chmura merged commit 765707f into main Jul 2, 2025
6 checks passed
@Jacob-Chmura
Jacob-Chmura deleted the dev/uniform_multi-hop branch July 2, 2025 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-Hop Uniform Sampling

2 participants