Add support to create temporal graphs #4819
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. |
…pport' into temporal_graph_support
ChuckHastings
added
the
improvement
Is this PR still a work-in-progress? |
No, this is ready for review. I updated the description. |
| @@ -40,6 +40,7 @@ namespace detail { | |||
| * @tparam edge_t Type of edge identifiers. Needs to be an integral type. | |||
| * @tparam weight_t Type of edge weights. Needs to be a floating point type. | |||
| * @tparam edge_type_t Type of edge type identifiers. Needs to be an integral type. | |||
| * @tparam edge_time_t The type of the edge time stamp. Needts to be an integral type. | |||
cpp/src/detail/groupby_and_count.cuh
Outdated
| }; | ||
|
|
||
| if (edge_property_count == 0) { | ||
| // TODO: Consider flipping the outer if and doing edge_property_count test first... |
There was a problem hiding this comment.
Any benefits of doing this? If not, we may just delete this. (and aren't we using FIXME instead of TODO?)
There was a problem hiding this comment.
Deleted this old message. I tend to use TODO for things I want to fix while developing and FIXME for things I want to defer. In this case, I did flip this to get the code that's here so the message is obsolete. My first attempt had been to have groupby_and_count_local_partition_by_minor as the outer if... which let me create the one _op functor or the other. But that resulted in too much duplication of code. So I implemented the comment in this TODO but forgot to delete it.
| @@ -202,7 +202,7 @@ neighbor_sample_impl(raft::handle_t const& handle, | |||
| ? std::make_optional(rmm::device_uvector<label_t>(0, handle.get_stream())) | |||
| : std::nullopt; | |||
|
|
|||
| for (auto edge_type_id = 0; edge_type_id < num_edge_types; edge_type_id++) { | |||
| for (edge_type_t edge_type_id = 0; edge_type_id < num_edge_types; edge_type_id++) { | |||
There was a problem hiding this comment.
Something very minor but should we better rename "edge_type_id" to "edge_type" as well?
| group_multi_edges(raft::handle_t const& handle, | ||
| rmm::device_uvector<vertex_t>&& edgelist_srcs, | ||
| rmm::device_uvector<vertex_t>&& edgelist_dsts, | ||
| rmm::device_uvector<value_t>&& edgelist_values, |
There was a problem hiding this comment.
I guess this wont' work if value_t is thrust::tuple. We may not need this for now, but should we better keep this as is to be future proof? Updating this code to support just a scalar value_t doesn't make this code much simpler...
There was a problem hiding this comment.
decltype(allocate_dataframe_buffer<edge_value_t>(0, rmm::cuda_stream_view{})) can now be dataframe_buffer_type_t<value_t> (https://github.com/rapidsai/cugraph/blob/branch-25.02/cpp/include/cugraph/utilities/dataframe_buffer.hpp#L91) to simplify the function definition a bit.
There was a problem hiding this comment.
Edited back to the original implementation with suggested improvements in the next push.
Adds two optional edge properties (
edge_start_timesandedge_end_times) that can be used to represent times when edges are active in the graph. This will support providing views of the graph at a particular time, or using edge times to control traversal in a reasonable time order.Marked as breaking. Most of the old APIs are still supported, but since I updated all of the tests to use the new APIs, we don't actually test the old APIs.