Fixed window temporal sampling - #5625
Conversation
…n use the new primitive
|
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
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
Quick reviews/questions about the API changes.
More reviews on the implementation part are in progress.
| time) */ | ||
| LAST /** Support last n behavior */ | ||
| FIXED_WINDOW, /** Apply the original per-seed time window at every hop */ | ||
| LAST = FIXED_WINDOW /** Deprecated alias for FIXED_WINDOW */ |
There was a problem hiding this comment.
What is this? Last-N is not a temporal sampling method, it is a neighbor selection method (as specified in neighbor_selection_t), right? Should we better delete this to avoid confusion?
There was a problem hiding this comment.
Or last-N or first-N are based on time stamps, not priorities (biases)?
There was a problem hiding this comment.
Last-N for the current use case is based on timestamps, but we already use timestamps to create biases, so we can easily implement with your new primitive. I think having it based on biases is a more flexible design anyway.
The LAST enum value was already defined, leaving this in place as deprecated to allow this to be a non-breaking API change. If a user previously called with LAST it would be valid, although it would have fast-failed as not implemented.
I'll change this so that it is not the same as FIXED_WINDOW (that's probably not correct) and instead returns a not implemented but deprecated error message. We'll actually delete the value in the next release.
| enum class neighbor_selection_t { | ||
| RANDOM = 0, /** Random selection. Uniform if no bias view is supplied, biased otherwise. */ | ||
| FIRST, /** Deterministically select the earliest edges. Not yet implemented. */ | ||
| LAST /** Deterministically select the latest edges. Not yet implemented. */ |
There was a problem hiding this comment.
Similar here, last-N or first-N are based on time stamps?
In this case, does it make sense to set neighbor_selection_t to LAST and temporal_sampling_comparison_t to STRICTLY_INCREASING?
There was a problem hiding this comment.
Yes, that might be a cleaner design. I'll look at that.
| neighbor_selection_t neighbor_selection, | ||
| std::optional<temporal_sampling_comparison_t> temporal_sampling_comparison, | ||
| sampling_flags_t sampling_flags, |
There was a problem hiding this comment.
Now we have neighbor_selection_t, temporal_sampling_comparison_t, and sampling_flags_t. But aren't the first two sampling flags (or sampling options)?
There was a problem hiding this comment.
Yes, I think combining them would make sense, let me look at that.
| * is required and @p fan_out contains one value per (hop, edge type). Otherwise, @p fan_out | ||
| * contains one value per hop. | ||
| * | ||
| * RANDOM selection samples uniformly when @p edge_bias_view is absent and samples according to |
There was a problem hiding this comment.
RANDOM=>neighbor_selection_t::RANDOM?
or something like
neighbor_selection_t dictates a neighbor selection method. RANDOM selection samples ... ?
And documentation for template and input parameters and return values are missing here.
There was a problem hiding this comment.
Will look at this
| return cugraph::c_api::run_algorithm(graph, functor, result, error); | ||
| auto const* options_cpp = | ||
| reinterpret_cast<cugraph::c_api::cugraph_sampling_options_t const*>(options); | ||
| return cugraph_neighbor_sample( |
There was a problem hiding this comment.
Any reason the biased wrappers call run_neighbor_sample() (https://github.com/rapidsai/cugraph/pull/5625/changes#diff-d268da7d4ce7574a739c9818704be440c1d2d633f087c39f8a73736b2f057859R746) directly while the uniform wrappers go through cugraph_neighbor_sample()? Better be more consistent about call chains?
It is a bit challenging to understand the call stack hierarchy.
There was a problem hiding this comment.
This all should go through cugraph_neighbor_sample, I'll fix that. I tried a few things and ended up with this model, missed getting rid of some of the run_neighbor_sample() calls.
The intention is to remove the old interfaces and have everything call cugraph_neighbor_sample, but I wanted to marek them as deprecated and leave the old methods in place for a release before we delete them.
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| // Built into libcugraph_common.so: sampling common TUs (gather_one_hop / sample_outgoing_edges) |
There was a problem hiding this comment.
This seems to describe a build/linkage requirement rather than the implementation itself. Would it be better to keep this explanation in CMakeLists.txt next to where these sources are added to CUGRAPH_COMMON_*?
There was a problem hiding this comment.
I'll move that.
2 participants
Add fixed window temporal sampling implementation, closes #5593
Also lays the groundwork for implementing last-n sampling, updating the API framework temporal sampling to make the last-n choice orthogonal to the temporal parameters.