Update symmetrization to use variant structures to reduce library size #5300
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
| std::optional<rmm::device_uvector<edge_time_t>>&& edgelist_edge_end_times, | ||
| bool reciprocal); | ||
| std::vector<arithmetic_device_uvector_t>> | ||
| add_reciprocal_edges( |
There was a problem hiding this comment.
I think this name is confusing.
This sounds like we are adding reciprocal edges to another edge list. But isn't this inspecting the input edge list and keeping only the reciprocal edges?
I think we need a clearer name.
And the function below...
Isn't removing non-reciprocal edges same to the previous symmetrize_edgelist function with reciprocal = true. The comment is saying the opposite.
So, there are two ways to symmetrize.
- Only keep the reciprocal edges. Remove all non-reciprocal edges.
- Add inverse edges for all non-reciprocal edges.
I think we'd better rename the functions to make this more intuitive...
I asked this to ChatGPT, and these are the names ChatGPT suggested...
make_reciprocal_edges() <= But I think these names are bad... Still confusing.
make_symmetric_edges()
symmetrize_edgelist_union() <= Sounds better... not sure reciprocal & intersection aligns 100%
symmetrize_edgelist_intersection()
keep_reciprocal_edges() <= If I need to pick one out of the three.. I may go with this.
add_inverse_edges()
But there might be better names than this... Just a starting point...
| * @brief Rule for combining edge properties when adding reciprocal edges. | ||
| */ | ||
| enum class edge_property_combining_rule_t { | ||
| PART_OF_UNIQUENESS, /// This property is part of the uniqueness of the edge. |
There was a problem hiding this comment.
Yeah... I am not sure about this... This is not really a combining rule...
And if this is selected for an edge property... how do we consider that in symmetrization.
Say edge ID is selected as "PART_OF_UNIQUENESS" property.
In (src, dst, ID) triplets, how do we symmetrize (3, 5, 1) and (5, 3, 1) with reciprocal = true/false? What about (3, 5, 1) and (5, 3, 2)?
I think we should make this cleaner and better document the expected behavior. Just reading the names, I can't properly guess how these functions will work.
| * @brief Add reciprocal edges and merge the edges. This was formerly called symmetrize_edgelist | ||
| * with reciprocal set to false. | ||
| * | ||
| * The input edge list is replicated as reciprocal edges. Then the original edges and the |
There was a problem hiding this comment.
I am wondering we should use "reciprocal" here or use "inverse" instead. I am not sure what "reciprocal" exactly means. Some quick search says it is a pair of edges going in both directions between a pair of vertices.
There was a problem hiding this comment.
I'm fine switching to inverse here if that's more intuitive to you. I suppose by the definition you found we're really creating reciprocal edges here by adding the inverse edge. So that's probably more correct.
| rmm::device_uvector<vertex_t>&& edgelist_srcs, | ||
| rmm::device_uvector<vertex_t>&& edgelist_dsts, | ||
| std::vector<arithmetic_device_uvector_t>&& edgelist_edge_properties, | ||
| std::vector<edge_key_selector_t> const& edge_key_selector, |
There was a problem hiding this comment.
We don't need std::vector<edge_value_combining_rule_t> const& edge_value_combining_rule here? If we have a reciprocal edges, are there no use-cases we need to combine edge properties? (e.g. update the edge weights to the average)
There was a problem hiding this comment.
Yes, I suppose we do. I hadn't considered that case. But if we have (1,2,1.0) and (2,1,2.0) then we would need to have the edge_value_combining_rule_t for that edge property to determine what value to assign to the pair of edges. In that case NONE would leave the edges unmodified (and different in each direction), ANY would arbitrarily pick one of them to make them match.
| FIRST, /** Use the value of the first edge property. */ | ||
| LAST, /** Use the value of the last edge property. */ | ||
| ANY, /** Use the value of any edge property. */ | ||
| NONE /** Do not combine the values. */ |
There was a problem hiding this comment.
This is just to keep the original values, right? Should we explain this in little more detail?
I initially thought we don't need NONE here as we now have edge_key_selector_t. But now I think this is still necessary as we may consider two different edges with the matching source/destination pairs as reciprocal but in some cases, we don't want to combine there edge properties. This may not be clear to everyone from the beginning.
There was a problem hiding this comment.
I'll add some text to clarify the different use cases.
2 participants
So far this just defines the proposed new symmetrization function signatures.