Last-n implementation using the new primitive - #5636
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
ChuckHastings
added
improvement
| times for STRICTLY_DECREASING and MONOTONICALLY_DECREASING. Does not | ||
| accept edge biases or with_replacement. Equal timestamps (and 64-bit | ||
| times that are not uniquely representable as double) may tie in | ||
| arbitrary order. */ |
There was a problem hiding this comment.
I am wondering LAST here is a right terminology or not.
Without any background and just encountering LAST, I will conjecture that we will pick the neighbors of last (largest, least recent, latest) time stamps.
But LAST here can mean the opposite based on the value of the temporal_sampling_comparison_t parameter.
We may need a better name, something like TOP_K, TOP_N, DETERMINISTIC, or ORDERED UNLESS LAST (or LAST_N) historically means picking the top N ones with the best scores, so people in the field won't get confused.
There was a problem hiding this comment.
I [technically the agent I asked] checked PyG, TGN, and DGL. There isn’t a single enum-name standard, but the closest ecosystem precedent is PyG’s temporal_strategy='last' (vs 'uniform'), added in pyg-team/pytorch_geometric#5576 (originally last_k). TGN uses --uniform as the inverse (default = most-recent), and DGL TGN examples use sampler_type='topk'. Literature calls it “most-recent sampling.” Names like TOP_K/DETERMINISTIC don’t appear as strategy enums in these libraries.
We used LAST to align with PyG and our legacy temporal_sampling_comparison_t::LAST. The intended meaning is “last-n along the temporal walk” (fanout-K edges ranked by start time), not unconditionally “latest timestamp.”
That said, cuGraph supports decreasing walks also, so the rank direction depends on temporal_sampling_comparison — an ambiguity PyG doesn’t expose. I’m happy to (a) keep LAST and make the docs explicit upfront, or (b) rename to LAST_N to signal “n neighbors” without implying a single time direction.
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
LGTM (one suggestions to improve performance)
| multi_edge_index.index()); | ||
| return std::get<rmm::device_uvector<edge_t>>(multi_edge_index).begin(); | ||
| }()) | ||
| : std::nullopt); |
There was a problem hiding this comment.
Can you call the edge_bucket constructor taking R-values? (https://github.com/rapidsai/cugraph/blob/main/cpp/include/cugraph/prims/edge_bucket.cuh#L67)
Then, we don't need to pay the cost (memory/compute) of insert.
There was a problem hiding this comment.
Updated the code to do that. I added a new method to the edge bucket to take back ownership of the device vectors.
I wonder if we would be better mimicking the key_bucket_view_t paradigm we use for the vertex buckets and create an edge_bucket_view_t that could just take the spans. We do have read-only use cases like this as well as read-write use cases.
… documentation updates to clarify the meaning of LAST in different time directions
3 participants
Had to make a minor change to the API, last-n needs to be orthogonal to the direction (increasing/decreasing) in order to support first-n (which is basically free if we keep the parameters orthogonal). I have marked this as non-breaking in spite of this change since we only defined the change within this release, so the only potential breakage would be developers using our nightlies.
Implements last-n using the new primitive. For int64_t times, the result might be approximate if edge times exceed 2**53 due to the conversion of the time to a double. If this becomes problematic we can explore alternative options.