Create graph with edge property values #2660
Conversation
seunghwak
commented
Sep 2, 2022
•
seunghwak
commented
- Move the CSR (+ DCSR, or CSC +DCSC) compression and adjacency list sort parts from graph_t to create_graph_from_edgelist; for the main code path, there is another graph_t constructor used in cython.cu, this constructor is still performing compress and sort, this function will be deleted once cython.cu retires. Currently, the compression and sort functions are moved to detail/structure_utils.cuh to be used in both create_graph_from_edgelist_impl.cuh and graph_impl.cuh.
- Added another graph_t constructor taking compressed and sorted CSR + DCSR graph data
- Updated the create_graph_from_edgelist function to ooptionally take edge IDs and types.
- Made few std::optional objects in multi-GPU struct/class non-optional (as they will always have a valid value in multi-GPU).
- Replace few more raw-pointers with raft::device_span
seunghwak
added
feature request
…alues other than weight_t
…reate_graph_with_edge_prop
seunghwak
changed the title
|
rerun tests |
|
Is this ready for me to test with #2629 ? |
Hopefully in few hours, I will mark this as ready-for-review once this is ready to be tested with #2629. I have locally tested, but we have two versions of create_graph_edgelist (one does not take edge IDs and types and one OPTIONALLY takes edge IDs and types, as the first can be implemented with the second, I want to remove the first). |
|
@alexbarghi-nv And I am currently assuming that edge types are 32 bit signed integers, and will this work for you? |
|
I was hoping they would be uint8 to save memory; right now we don't have any use cases with anything more than a handful of edge types. |
|
But if they need to be int32 that's ok, it will just not be very memory-efficient |
OK, then, let's stick to int32_t at this moment, but once we find this becomes a memory bottleneck, we will get back to uint8_t. We can instantiate this for both uint8_t and uint32_t but we're getting some complaints about compile time and binary size, so we may want to avoid instantiating for too many type combinations. uint8 may not be sufficient for some future use cases but if this limits the maximum size we can run, we can use uint8 instead. |
|
Ok, sounds good. I'll update my code to use int32 |
|
@alexbarghi-nv OK, this PR is now ready for testing. |
…reate_graph_with_edge_prop
cpp/src/c_api/graph_sg.cpp
Outdated
| cugraph::create_graph_from_edgelist<vertex_t, | ||
| edge_t, | ||
| weight_t, | ||
| int32_t, |
There was a problem hiding this comment.
This hard-codes int32_t, which is fine for now (as you've discussed with @alexbarghi-nv. Ultimately we should probably allow this to be arbitrary, but I think we can defer that to later.
There was a problem hiding this comment.
What about doing using edge_type_t = int32_t then passing edge_type_t? I guess this will make the intention clearer.
There was a problem hiding this comment.
I think that would be a bit better. That would make the references easier to isolate when we ultimately add support for variable types.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-22.10 #2660 +/- ##
===============================================
Coverage ? 60.11%
===============================================
Files ? 112
Lines ? 6150
Branches ? 0
===============================================
Hits ? 3697
Misses ? 2453
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@gpucibot merge |
