Repository navigation
Heterogeneous temporal sampling fixes - #5691
Merged
rapids-bot[bot] merged 6 commits intoOct 6, 2026
Merged
rapids-bot[bot] merged 6 commits into
rapids-bot[bot] merged 6 commits into
Conversation
…but for lastn resulting in uniform random selection
|
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. |
alexbarghi-nv
approved these changes
Oct 5, 2026
alexbarghi-nv
left a comment
alexbarghi-nv
left a comment
Member
There was a problem hiding this comment.
Looks good functionally - I'll defer to Seunghwa on performance
ChuckHastings
added
improvement
ChuckHastings
marked this pull request as ready for review
October 6, 2026 16:12
seunghwak
approved these changes
Oct 6, 2026
seunghwak
left a comment
seunghwak
left a comment
Contributor
There was a problem hiding this comment.
Minor complaints about documentation but otherwise LGTM.
| * increasing modes, earlier times (last in decreasing order) for decreasing modes. | ||
| * int32 start times rank exactly over the full signed range; int64 ranks exactly | ||
| * on [-2^52, 2^52 - 1]. Outside that int64 range, ordering is preserved but ties | ||
| * A start time equal to the minimum of the timestamp type is ranked by a uniform key in |
Contributor
There was a problem hiding this comment.
minimum=>numeric_limits<time_stamp_t>::lowest() to be consistent with the above documentation?
| * decreasing order) for decreasing comparisons. LAST requires temporal sampling, ignores | ||
| * edge biases, and rejects with-replacement. int32 start times rank exactly over the full | ||
| * signed range; int64 ranks exactly on [-2^52, 2^52 - 1] (typical unix | ||
| * decreasing order) for decreasing comparisons. An edge start time equal to the minimum |
Contributor
There was a problem hiding this comment.
We may pick minium or numeric_limits<time_stamp_t>::lowest() and use consistently rather than mixing them.
| * Sampling is non-temporal unless cugraph_sampling_set_temporal_sampling_comparison has been | ||
| * called on @p sampling_options; all temporal arguments must then be NULL. When temporal, | ||
| * called on @p sampling_options; all temporal arguments must then be NULL. When temporal, an edge | ||
| * start time equal to the minimum value of the timestamp type (INT32_MIN or INT64_MIN) means the |
Contributor
There was a problem hiding this comment.
Same here. We are mixing mininum and numeric_limits<time_stamp_t>::lowest in documentation.
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Temporal sampling for heterogeneous graphs needs to support the idea that some types won't have edge times.
This PR changes the logic for times. Since a time is required for every edge in our data structure, we can use a sentinel value to indicate that no time is specified. We chose the value std::numeric_limits<edge_time_t>::lowest() as this sentinel value.
If an edge time is set to that value then the software will now treat that edge as if it passes any time filter. We also added logic for special cases: