-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new subsitute_subgraph() method to graph classes #823
base: main
Are you sure you want to change the base?
Conversation
This commit adds a new method, substitute_subgraph(), to the PyGraph and PyDiGraph classes. It is used to replace a subgraph in the graph with an external graph.
Pull Request Test Coverage Report for Build 7573414576
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, overall.
I spent some time trying to think of ways we could do the cycle check prior to making edits to the graph, if you think it's worth the effort. I believe we'd need to check the subgraph for all paths between incoming nodes to outgoing nodes, and then the original graph for paths going the other way between the corresponding IO nodes we're removing that would still be in tact after the replacement (no middle hops using nodes we're removing). We also might need special consideration for the case where input_node_map
maps multiple indices from the original graph to the same new replacement node, since that would be very similar to contract_nodes
, though I haven't thought it through.
I think we can just proceed with this as is in terms of the failure case leaving things in a bad state for now, and add preemptive checking for cycles in the future since that wouldn't be a breaking change.
@@ -227,9 +227,10 @@ impl PyDiGraph { | |||
p_index: NodeIndex, | |||
c_index: NodeIndex, | |||
edge: PyObject, | |||
force: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name force
for this parameter seems a bit misleading to me. In the context of this method's name, I would expect that force would mean to add the edge, even if it violates properties of the graph (e.g. introduces cycles). I'm also not a fan of us needing to pass a new parameter at all of the callsites for _add_edge
.
Instead, what would you think about adding a new method called something like _error_if_would_cycle
that takes the indices in question, and contains code moved from _add_edge
which does the cycle check and error construction:
fn _error_if_would_cycle(&mut self, p_index: NodeIndex, c_index: NodeIndex) -> PyResult<()> {
// Only check for a cycle (by running has_path_connecting) if
// the new edge could potentially add a cycle
let cycle_check_required = is_cycle_check_required(self, p_index, c_index);
let state = Some(&mut self.cycle_state);
if cycle_check_required
&& algo::has_path_connecting(&self.graph, c_index, p_index, state)
{
return Err(DAGWouldCycle::new_err("Adding an edge would cycle"));
}
Ok()
}
We could then call this from _add_edge
if self.check_cycle
is true
, which preserves the current expectations of callers, and call it manually followed by a call to add_edge_no_cycle_check
from methods like substitute_subgraph
which offer cycle detection irrespective of whether or not self.check_cycle
is enabled.
This commit adds a new method, substitute_subgraph(), to the PyGraph and PyDiGraph classes. It is used to replace a subgraph in the graph with an external graph.