Add support for other temporal comparisons #5283
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
| STRICTLY_DECREASING, /** Time strictly decreasing (each time is before the previous one) */ | ||
| MONOTONICALLY_DECREASING, /** Time monotonically decreasing (could have multiple edges with same | ||
| time) */ | ||
| LAST /** Support last n behavior */ |
There was a problem hiding this comment.
LAST /** Support last n behavior */
This sounds vague to me.
Isn't this something like NUM_COMPARISON_TYPES? (basically the number of supported temporal sampling comparison methods/modes/types... and so o n)
There was a problem hiding this comment.
This is not the end of the enum. It's actually to support a feature that will be implemented later called "last n".
If LAST is specified, the software will (eventually, not part of this PR) use the fanout value (call it n) and instead of randomly selecting n edges that leave this vertex it will select the n edges with the largest time stamps. You can think of this as sorting the outgoing edges by time and selecting the "last n" of them.
This is a feature that we'll add later (perhaps later in 25.12, perhaps not until 26.01, not sure of the priorities and how far we'll progress).
| temporal_sampling_comparison = | ||
| cugraph::temporal_sampling_comparison_t::MONOTONICALLY_DECREASING; | ||
| break; | ||
| default: CUGRAPH_FAIL("Invalid temporal sampling comparison type"); |
There was a problem hiding this comment.
auto temporal_sampling_comparison = options_.temporal_sampling_comparison_;
Won't this work?
There was a problem hiding this comment.
We're converting between types here. I'm hesitant to assume that translating from the C enum definition in the .h file to the C++ enum class definition in the .hpp file is guaranteed to work by assignment. This was my simple solution.
| options_.dedupe_sources_, | ||
| options_.with_replacement_}, | ||
| options_.with_replacement_, | ||
| temporal_sampling_comparison}, |
There was a problem hiding this comment.
Or can we just call this with options_.temporal_sampling_comparison_? Why are we creating a temporary?
There was a problem hiding this comment.
Type conversion as mentioned above
| return (edge_time < key_time); | ||
| case temporal_sampling_comparison_t::MONOTONICALLY_DECREASING: | ||
| return (edge_time <= key_time); | ||
| } |
There was a problem hiding this comment.
assert(false); // never be reached I assume this part should never be reached...
There was a problem hiding this comment.
I'll make that change.
| return (edge_time < key_time); | ||
| case temporal_sampling_comparison_t::MONOTONICALLY_DECREASING: | ||
| return (edge_time <= key_time); | ||
| } |
There was a problem hiding this comment.
Same here.
assert(false); // never be reached
There was a problem hiding this comment.
I'll make that change
Closes #5040.
Update temporal sampling to support the following variations:
This PR includes C++ tests and C API tests.
Marked as breaking as we add a parameter to the C++ temporal sampling function, although it can be defaulted so it's only breaking for code that specifies
do_expensive_check.