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.
2 participants
So far this just defines the proposed new symmetrization function signatures.