SP-ization #1562
SP-ization #1562
Conversation
…m SerialParallelDecomposition
Marsella8
left a comment
Marsella8
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 78 files reviewed, 1 unresolved discussion (waiting on @lockshaw)
lib/utils/test/src/utils/graph/series_parallel/sp_ization/spanish_algo.cc line 23 at r1 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("spanish_algo - subcomponents") { SUBCASE("add_dummy_nodes") {
how should I test graphs where nodes are added? Here I check for some properties but it's obviously non-exhaustive
Marsella8
left a comment
Marsella8
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 78 files reviewed, 3 unresolved discussions (waiting on @lockshaw)
lib/utils/include/utils/graph/series_parallel/sp_ization/work_preserving_sp_ization.h line 64 at r1 (raw file):
*the SP-ized graph. **/ SeriesParallelDecomposition cost_aware_stratum_sync_sp_ization(
old cost-aware algo, will leave it in for benchmarking purposes though we'll probably remove it eventually
bin/sp_ization_benchmarking/nasnet_bench_graph_generator.h line 1 at r1 (raw file):
// For context, see https://arxiv.org/abs/1902.09635 &&
Benchmarking code is a bit sloppy but it's onyl temporary, should be fine(?)
lockshaw
left a comment
lockshaw
left a comment
There was a problem hiding this comment.
Reviewed 44 of 78 files at r1, 10 of 19 files at r2, 1 of 1 files at r3, 2 of 3 files at r4, all commit messages.
Reviewable status: 56 of 78 files reviewed, 34 unresolved discussions (waiting on @Marsella8)
lib/utils/include/utils/graph/series_parallel/sp_ization/work_preserving_sp_ization.h line 64 at r1 (raw file):
Previously, Marsella8 wrote…
old cost-aware algo, will leave it in for benchmarking purposes though we'll probably remove it eventually
Sounds good, can you include this information in the actual comment itself? It's useful to have information like this (i.e., how all the various algorithms relate to each other and what our future plans are for them) in the code itself
lib/utils/test/src/utils/graph/series_parallel/sp_ization/spanish_algo.cc line 23 at r1 (raw file):
Previously, Marsella8 wrote…
how should I test graphs where nodes are added? Here I check for some properties but it's obviously non-exhaustive
I'd recommend isomorphism checking, similar to what we do with dataflow graphs
lib/utils/include/utils/graph/digraph/algorithms/get_bottlenecks.h line 19 at r4 (raw file):
bottleneck if and only if it's the unique source / sink of the graph. */ std::unordered_set<Node> get_bottlenecks(DiGraphView const &g);
What is the difference between bottlenecks and dominators?
lib/utils/include/utils/graph/digraph/algorithms/get_ancestors.h line 14 at r2 (raw file):
* @note `n` is not considered to be its own descendant, and is thus not *included in the returned set. **/
Suggestion:
/**
* @brief Computes the set of all ancestors of a given node `n` in a directed
* graph, which is the set of all nodes `m` for which a directed path from `n` to
* `m` exists.
*
* @note `n` is not considered to be its own ancestor, and is thus not
* included in the returned set.
*/lib/utils/include/utils/graph/digraph/algorithms/get_descendants.h line 10 at r4 (raw file):
/** * @brief Computes the set of all descendants of a given node in a directed *graph.
Suggestion:
* graph.lib/utils/include/utils/graph/digraph/algorithms/get_descendants.h line 13 at r4 (raw file):
* * @note `starting_node` is not considered to be its own descendant, and is thus *not included in the returned set.
Suggestion:
* not included in the returned set.lib/utils/include/utils/graph/digraph/algorithms/get_lowest_common_ancestors.h line 35 at r4 (raw file):
* LCA, or a set of nodes as LCA. */ std::optional<std::unordered_set<Node>>
Why does this return a std::optional? What does returning std::nullopt signify?
Code quote:
std::optional<std::unordered_set<Node>>lib/utils/include/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.h line 18 at r4 (raw file):
* @note The root has a path length of 1. g must be acyclic. */ std::unordered_map<Node, int>
Suggestion:
std::unordered_map<Node, nonnegative_int>lib/utils/include/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.h line 32 at r4 (raw file):
*/ std::unordered_map<Node, float> get_weighted_longest_path_lengths_from_root( DiGraphView const &g, std::unordered_map<Node, float> const &node_costs);
Are negative node costs allowed by this function?
Code quote:
DiGraphView const &g, std::unordered_map<Node, float> const &node_costs);lib/utils/include/utils/graph/series_parallel/series_parallel_decomposition.h line 23 at r4 (raw file):
bool is_empty(SeriesSplit const &serial); bool is_empty(ParallelSplit const ¶llel); bool is_empty(SeriesParallelDecomposition const &sp);
Should be changed to be defined for NonNormalSeriesParallelDecomposition
Code quote:
bool is_empty(SeriesParallelDecomposition const &sp);lib/utils/include/utils/graph/series_parallel/series_parallel_decomposition.h line 32 at r4 (raw file):
* multiple times */ size_t num_nodes(SeriesParallelDecomposition const &sp);
Suggestion:
nonnegative_int num_nodes(SeriesParallelDecomposition const &sp);lib/utils/include/utils/graph/series_parallel/sp_ization/node_role.enum.toml line 2 at r4 (raw file):
namespace = "FlexFlow" name = "NodeRole"
What is this? A docstring would be quite helpful here to add some context for what this enum represents and what it's used for
lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 20 at r4 (raw file):
get_node_counter_map(ParallelSplit const ¶llel); std::unordered_map<Node, size_t> get_node_counter_map(SeriesParallelDecomposition const &sp);
Seems weird having all but the last of these as public functions--what about making the first three lambdas/static?
lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 20 at r4 (raw file):
get_node_counter_map(ParallelSplit const ¶llel); std::unordered_map<Node, size_t> get_node_counter_map(SeriesParallelDecomposition const &sp);
Suggestion:
std::unordered_map<Node, nonnegative_int>
get_node_counter_map(SeriesParallelDecomposition const &sp);lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 20 at r4 (raw file):
get_node_counter_map(ParallelSplit const ¶llel); std::unordered_map<Node, size_t> get_node_counter_map(SeriesParallelDecomposition const &sp);
Suggestion:
get_node_num_occurences_map(SeriesParallelDecomposition const &sp);lib/utils/include/utils/graph/series_parallel/series_parallel_metrics.h line 38 at r4 (raw file):
* */ int num_dependencies(SeriesParallelDecomposition const &sp);
Suggestion:
nonnegative_int num_dependencies(SeriesParallelDecomposition const &sp);lib/utils/include/utils/graph/series_parallel/sp_ization/spanish_algo.h line 23 at r4 (raw file):
std::unordered_map<Node, NodeRole> const &node_roles); SeriesParallelDecomposition spanish_strata_sync(DiGraph g);
Can we get some documentation of the functions in this header, ideally accompanied by some corresponding links to the paper?
lib/utils/test/src/utils/graph/digraph/algorithms/is_acyclic.cc line 55 at r4 (raw file):
CHECK(is_acyclic(g)); } SUBCASE("traversal with root") {
Why "traversal"? I'm not sure what it means in this context
Code quote:
SUBCASE("traversal with root") {lib/utils/test/src/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.cc line 14 at r4 (raw file):
std::vector<Node> n = add_nodes(g, 5); std::vector<DirectedEdge> edges = { DirectedEdge{n[0], n[1]},
Prefer .at for bounds-checking
Suggestion:
DirectedEdge{n.at(0), n[1]},lib/utils/test/src/utils/graph/digraph/algorithms/get_longest_path_lengths_from_root.cc line 33 at r4 (raw file):
} TEST_CASE("get_longest_path_lengths_from_root - more complex graph") {
Prefer TEST_CASE with nested SUBCASEs
Code quote:
TEST_CASE("get_longest_path_lengths_from_root - more complex graph") {lib/utils/test/src/utils/graph/series_parallel/digraph_generation.cc line 54 at r4 (raw file):
} SUBCASE("Mixed Serial-Parallel") {
Suggestion:
SUBCASE("Mixed Series-Parallel") {lib/utils/test/src/utils/graph/series_parallel/digraph_generation.cc line 65 at r4 (raw file):
} SUBCASE("Mixed Parallel-Serial") {
Suggestion:
SUBCASE("Mixed Parallel-Series") {lib/utils/test/src/utils/graph/series_parallel/digraph_generation.cc line 93 at r4 (raw file):
CHECK(num_edges(result) == 4); CHECK(get_sources(result).size() == 1); CHECK(get_sinks(result).size() == 1);
Probably better to check the full graph structure using isomorphism checking
Code quote:
CHECK(num_nodes(result) == 4);
CHECK(num_edges(result) == 4);
CHECK(get_sources(result).size() == 1);
CHECK(get_sinks(result).size() == 1);lib/utils/test/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 11 at r4 (raw file):
TEST_CASE("get_lowest_common_ancestors") { DiGraph g = DiGraph::create<AdjacencyDiGraph>();
Add a check for the case where get_lowest_common_ancestors returns std::nullopt?
lib/utils/test/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 18 at r4 (raw file):
std::unordered_set<Node> result = get_lowest_common_ancestors(g, {n.at(0)}).value(); CHECK(correct == result);
Suggestion:
std::optional<std::unordered_set<Node>> correct = {n.at(0)};
std::optional<std::unordered_set<Node>> result =
get_lowest_common_ancestors(g, {n.at(0)});
CHECK(correct == result);lib/utils/test/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 38 at r4 (raw file):
correct = {n.at(2)}; result = get_lowest_common_ancestors(g, {n.at(2)}).value(); CHECK(correct == result);
Ideally justify these examples with SUBCASEs. Ideally every correct/result should be declared once in its own SUBCASE (doesn't have to always hold, but is a good heuristic)
Code quote:
std::unordered_set<Node> correct = {n.at(0)};
std::unordered_set<Node> result =
get_lowest_common_ancestors(g, {n.at(1), n.at(2)}).value();
CHECK(correct == result);
correct = {n.at(1)};
result = get_lowest_common_ancestors(g, {n.at(1)}).value();
CHECK(correct == result);
correct = {n.at(2)};
result = get_lowest_common_ancestors(g, {n.at(2)}).value();
CHECK(correct == result);lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 15 at r4 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("critical_path_preserving_sp_ization") {
Honestly some rapidcheck tests might be nice here in addition to the manual tests, as there's a clear property that you can check that you would like to hold for all acyclic dags
lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 17 at r4 (raw file):
TEST_CASE("critical_path_preserving_sp_ization") { SUBCASE("Sample Graph #1") {
It would be nice to have stronger justification for this testcase than "Sample Graph #1"--what is this subcase checking? Why did you choose this graph? How is it different from "Sample Graph #2"?
lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 55 at r4 (raw file):
CHECK(correct == result); } SUBCASE("work cost") {
This doesn't seem to check any properties of critical_path_preserving_sp_ization beyond what the "structure" subcase checks?
lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 286 at r4 (raw file):
} } SUBCASE("Transitive Reduction") {
What does htis mean as a testcase? It doesn't seem like it's testing transitive reduction, so it's a bit oddly named.
lib/utils/include/utils/graph/series_parallel/normalize_sp_decomposition.h line 11 at r4 (raw file):
* @brief Recursively normalizes a SeriesParallelDecomposition. * * @details This function performs the following semantic substitutions:
I would rather this be a structural property of SeriesParallelDecomposition. Can you create some additional type (e.g., NonNormalSPDecomposition) and then make this function be SeriesParallelDecomposition normalize_sp_decomposition(NonNormalSPDecomposition const &)?
lib/utils/test/src/utils/graph/series_parallel/normalize_sp_decomposition.cc line 9 at r4 (raw file):
TEST_SUITE(FF_TEST_SUITE) { TEST_CASE("normalize_sp_decomposition") { Node n1 = Node(1);
Prefer bracket initialization
Suggestion:
Node n1 = Node{1};lib/utils/src/utils/containers/invert_map.cc line 1 at r2 (raw file):
#include "utils/containers/invert_map.h"
Add an explicit specialization using value_type
lib/utils/test/src/utils/graph/series_parallel/get_ancestors.cc line 11 at r4 (raw file):
TEST_CASE("get_ancestors") { std::vector<Node> n = { Node(0), Node(1), Node(2), Node(3), Node(4), Node(5), Node(6), Node(7)};
Prefer bracket initialization
Suggestion:
Node{0}, Node(1), Node(2), Node(3), Node(4), Node(5), Node(6), Node(7)};
lockshaw
left a comment
lockshaw
left a comment
There was a problem hiding this comment.
@lockshaw partially reviewed 55 files and all commit messages, made 25 comments, and resolved 6 discussions.
Reviewable status: 84 of 92 files reviewed, 51 unresolved discussions (waiting on @Marsella8).
lib/utils/test/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 17 at r4 (raw file):
Previously, lockshaw (Colin Unger) wrote…
It would be nice to have stronger justification for this testcase than "Sample Graph #1"--what is this subcase checking? Why did you choose this graph? How is it different from "Sample Graph #2"?
Note that those should be #1 and #2, it helpfully expanded the corresponding PRs 😂
lib/utils/src/utils/graph/digraph/algorithms/get_descendants.cc line 16 at r5 (raw file):
std::unordered_set<Node> descendants; std::stack<Node> to_visit;
Don't we already have bfs and dfs functions? Is there a reason we don't just reuse those to implement this?
lib/utils/src/utils/graph/digraph/algorithms/is_acyclic.cc line 21 at r5 (raw file):
}); // recursively explore a given node and all its successors: if, while
How does this return the correct result for the following graph?
digraph {
a -> b;
a -> c;
b -> d;
c -> d;
}
If I run in the order [a, b, c, d], then it seems that by the time I get to c d has already been set to BEING_EXPLORED by processing b, and so if I'm reading it correctly I'd get a cycle?
Also, why are we replacing the old implementation? Did we find a bug in it that wasn't easily fixable, or is it for other reasons?
lib/utils/include/utils/graph/digraph/algorithms/get_edges.h line 8 at r5 (raw file):
namespace FlexFlow { size_t num_edges(DiGraphView const &);
Why? Feels like this is an unnecessary layer on top of get_edges? Not really against adding it, just curious
If you do want to keep it, move it to it's own header file and have it return a nonnegative_int
lib/utils/src/utils/graph/digraph/algorithms/transitive_reduction.cc line 35 at r5 (raw file):
DiGraph transitive_reduction(DiGraphView const &g) { // Logic dropped down to raw adjacency matrix for performance. // The version going through the full graph abstraction was
Any reason for the change?
lib/utils/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 32 at r5 (raw file):
if (common_ancestors.empty()) { return common_ancestors; }
Minor: Arguably more clear
Suggestion:
if (common_ancestors.empty()) {
return std::unordered_set<Node>{};
}lib/utils/src/utils/graph/digraph/algorithms/get_lowest_common_ancestors.cc line 40 at r5 (raw file):
return depth_levels.at(n) == largest_depth_for_common_ancestors; }); }
Please add more blank lines in your code--write now it feels like reading an essay/book without paragraph breaks (i.e., doable, but requiring unnecessary mental effort)
Code quote:
std::optional<std::unordered_set<Node>>
get_lowest_common_ancestors(DiGraphView const &g,
std::unordered_set<Node> const &nodes) {
assert(is_acyclic(g));
assert(is_subseteq_of(nodes, get_nodes(g)));
if (num_nodes(g) == 0 || nodes.size() == 0) {
return std::nullopt;
}
std::unordered_set<std::unordered_set<Node>> ancestors =
transform(nodes, [&](Node const &n) {
return set_union(get_ancestors(g, n), {n});
});
std::unordered_set<Node> common_ancestors = intersection(ancestors).value();
if (common_ancestors.empty()) {
return common_ancestors;
}
std::unordered_map<Node, nonnegative_int> depth_levels =
get_longest_path_lengths_from_root(g);
nonnegative_int largest_depth_for_common_ancestors = maximum(transform(
common_ancestors, [&](Node const &n) { return depth_levels.at(n); }));
return filter(common_ancestors, [&](Node const &n) {
return depth_levels.at(n) == largest_depth_for_common_ancestors;
});
}lib/utils/src/utils/containers/invert_map.cc line 9 at r5 (raw file):
template std::unordered_map<V, std::unordered_set<K>> invert_map(std::unordered_map<K, V> const &);
FYI this is quite similar to some of the logic in the recently-added OneToMany and ManyToOne classes, might not hurt to take a look at those just in case they might help you simplify some code
Code quote:
template std::unordered_map<V, std::unordered_set<K>>
invert_map(std::unordered_map<K, V> const &);bin/sp_ization_benchmarking/nasnet_bench_graph_generator.h line 2 at r5 (raw file):
// For context, see https://arxiv.org/abs/1902.09635 && // https://github.com/google-research/nasbench/blob/master/nasbench/api.py
May as well just use doxygen syntax while we're at it
Suggestion:
/**
* @brief <Pietro add me>
*
* For context, see https://arxiv.org/abs/1902.09635 &&
* https://github.com/google-research/nasbench/blob/master/nasbench/api.py
*/lib/utils/src/utils/graph/digraph/algorithms/get_bottlenecks.cc line 14 at r5 (raw file):
std::unordered_set<Node> get_bottlenecks(DiGraphView const &g) { std::unordered_set<Node> bottlenecks; assert(is_acyclic(g));
Using ASSERT from libassert gives us nicely-printed stack traces and more configurable behavior: https://github.com/jeremy-rifkin/libassert
Suggestion:
ASSERT(is_acyclic(g));lib/utils/src/utils/graph/digraph/algorithms/get_bottlenecks.cc line 23 at r5 (raw file):
bottlenecks.insert(n); } }
Minor: could be done with a filter I think, for arguably better readability
Code quote:
for (Node const &n : get_nodes(g)) {
DiGraphView subgraph = get_subgraph(g, set_difference(get_nodes(g), {n}));
if (get_weakly_connected_components(subgraph).size() == 2) {
bottlenecks.insert(n);
}
}lib/utils/src/utils/graph/digraph/algorithms/is_tree.cc line 11 at r5 (raw file):
bool is_tree(DiGraphView const &g) { assert(num_nodes(g) > 0);
Suggestion:
ASSERT(num_nodes(g) > 0);lib/utils/include/utils/graph/series_parallel/sp_ization/flexible_algo.h line 16 at r5 (raw file):
SeriesParallelDecomposition flexible_sync(DiGraphView const &g,
Minor: Is this the cost-aware algorithm? We might want to come up with a slightly less ambiguous name at some point...
Code quote:
flexible_sync(DiGraphView lib/utils/src/utils/graph/series_parallel/series_parallel_metrics.cc line 67 at r5 (raw file):
} float critical_path_cost(SeriesSplit const &serial,
I don't love having all of these overloads part of the public interface, as it makes the whole thing about difficult to approach. Is it possible to restrict some of these to being either static or lambdas, and then only expose the high-level functions (e.g., the ones that operate over SeriesParallelDecomposition, not the ones that operator on Node)
lib/utils/src/utils/graph/series_parallel/series_parallel_metrics.cc line 46 at r5 (raw file):
std::unordered_map<Node, nonnegative_int> get_node_counter_map(SeriesParallelDecomposition const &sp) {
Can we get a clearer name for this? node_counter_map is not really self-describing--maybe something like get_num_occurrences_of_nodes?
Code quote:
get_node_counter_map(SeriesParallelDecomposition lib/utils/src/utils/graph/series_parallel/series_parallel_metrics.cc line 98 at r5 (raw file):
} int num_dependencies(SeriesParallelDecomposition const &sp) {
Suggestion:
nonnegative_int num_dependencies(SeriesParallelDecomposition const &sp) {lib/utils/src/utils/graph/series_parallel/get_ancestors.cc line 37 at r5 (raw file):
} static bool perform_traversal(ParallelSplit const ¶llel,
I'm having difficulty understanding this code--could this be changed to a slightly more informative name than "perform_traversal", or if not could at least a couple comments be added to help guide the reader through this code? It's not aided by the fact that the signature (both outputing a boolean and then modifying a std::unordered_set) is rather complicated, so inferring the behavior from the type signature is rather difficult
Code quote:
static bool perform_traversal(ParallelSplit const ¶llel,lib/utils/src/utils/graph/series_parallel/sp_ization/critical_path_preserving_sp_ization.cc line 118 at r5 (raw file):
SeriesParallelDecomposition critical_path_preserving_sp_ization(DiGraphView const &g) {
What are all the functions in this file? I'm confused by the names, since naive stratum sync preserves critical path, but that doesn't seem to be what these are, so critical_path_preserving_sp_ization is a really confusing name
lib/utils/src/utils/graph/series_parallel/normalize_sp_decomposition.cc line 24 at r5 (raw file):
SeriesParallelDecomposition normalize_sp_decomposition(SeriesSplit const &serial) {
Would it be possible to instead have a separate type for non-normalized decompositions, so that whether or not something is normalized can be determined from the type declaration? I'm not always against having these properties be dynamic (e.g., see dynamic graph in task-spec), but I'm not really seeing a reason it needs to be dynamic here. That would also create a nice place to have documentation of what you mean by "normalization", for people new to the code
lib/utils/src/utils/graph/series_parallel/sp_ization/up_down_partition.cc line 9 at r5 (raw file):
namespace FlexFlow { std::unordered_set<Node> get_up_frontier(DiGraph const &sp,
Can you add a brief readme somehwere describing some of the terminology and high-level of the code here? There's terminology that I'm not familiar with (e.g., "up frontier") and it would be nice to have some documentation. Ideally also some doxygen docstrings so that the docs are closer to the code and then link to the readme where necessary, but having some documentation is more important than making that documentation perfect
Code quote:
get_up_frontier(DiGraph lib/utils/src/utils/graph/series_parallel/digraph_generation.cc line 88 at r5 (raw file):
} DiGraph digraph_from_sp_decomposition(ParallelSplit const ¶llel) {
It's confusing having all of these exposed under the same name. Either make them internal (either static or lambdas) or give them more accurate names (e.g., digraph_from_parallel_split). In general, I don't mind using overloads as part of an implementation but I don't like exposing them as APIs to the rest of the codebase
Code quote:
digraph_from_sp_decomposition(ParallelSplit lib/utils/src/utils/graph/series_parallel/digraph_generation.cc line 94 at r5 (raw file):
}); return parallel_composition( transform(children, [](auto const child) -> DiGraphView {
Minor: overall a few less auto's in your lambda arguments would be nice--I know it's convenient for writing the code but it forces me to do type inference in my head. You don't have to never do them, but maybe try to include a bit more explicit type information when it's not too inconvenient
Suggestion:
transform(children, [](auto const &child) -> DiGraphView {lib/utils/src/utils/graph/series_parallel/digraph_generation.cc line 99 at r5 (raw file):
} DiGraph digraph_from_sp_decomposition(SeriesParallelDecomposition const &sp) {
Does this preserve node ids, or does the returned graph have different node ids than those contained in sp?
lib/utils/test/src/test_algorithms.cc line 79 at r5 (raw file):
} SUBCASE("get_dominators") {
We already have dedicated test files for a bunch of these (e.g., https://github.com/flexflow/flexflow-train/blob/f71c384ce5decd5a30859e39f75f1418cb76d91c/lib/utils/test/src/utils/graph/digraph/algorithms/get_dominators.cc#). Why was this file added?
lib/utils/src/utils/graph/series_parallel/sp_ization/node_role.cc line 17 at r5 (raw file):
} DiGraph delete_nodes_of_given_role(
Maybe we could give this a more accurate name than "delete" since it's doing more than deleting as it's also inserting edges? Personally I like "explode", as it both gives the sense of the outward-radiating edges, and the fact that once a node is exploded it's no longer there. The other option I can think of would be contract_out_node, which I think also gives a good sense of the underlying operation just from the name
Code quote:
DiGraph delete_nodes_of_given_role(
lockshaw
left a comment
lockshaw
left a comment
There was a problem hiding this comment.
@lockshaw partially reviewed 55 files and all commit messages, made 25 comments, and resolved 6 discussions.
Reviewable status: 84 of 92 files reviewed, 51 unresolved discussions (waiting on @Marsella8).
2 participants
Description of changes:
Related Issues:
Linked Issues:
Issues closed by this PR:
Before merging:
This change is