feat!: Boundary order checking

[aborgna-q]

Implements #161 as described in CQCL/hugr#1656.

Once this is merged, we should be able to optimise the convex checker to only test the edges returned by PortOrdering::missing_pairs. When the original order is stronger than the other in a rewrite, convex checking can be skipped altogether.

BREAKING CHANGE: Subgraph::new_subgraph now takes a Boundary as argument.

[codecov]

Codecov Report

Attention: Patch coverage is 94.76744% with 18 lines in your changes missing coverage. Please review.

Project coverage is 83.23%. Comparing base (f358376) to head (c35aeb5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/boundary.rs	95.98%	10 Missing and 1 partial ⚠️
src/algorithms/toposort.rs	69.56%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   82.29%   83.23%   +0.93%     
==========================================
  Files          23       24       +1     
  Lines        5830     6091     +261     
  Branches     5830     6091     +261     
==========================================
+ Hits         4798     5070     +272     
+ Misses        956      945      -11     
  Partials       76       76              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lmondada]

Very cool, thanks Agustin, mostly minor comments. The only part that confuses me is what arguments exactly fn port_ordering expects. See my comment below.

[lmondada]

I am confused here whether the argument graph: &impl LinkView needs to be a graph of the bounded subgraph exactly, or it can be a larger graph that included the subgraph of interest.

The fact that on line 176 you filter for nodes with 0 incoming neighbours makes me think that this will only work if the bounded graph is the entire graph -- but in that case, isn't the boundary already specified by the set of disconnected ports?

From the user's perspective, I would find passing a graph for which the region of interest is only a subgraph a simpler interface to use -- otherwise I'd expect that in most cases the user will have to construct a Subgraph struct before being able to call this function.

If the intention is to let user pass large graphs of which only a subgraph is of interest, then doing a toposort of the entire graph on line 180 seems wasteful.

Let me know if I misunderstood something!

[aborgna-q]

There was a bug in my code, but the main problem here are nodes that cannot be reached from the boundary inputs.

For example, consider the boundary input: [0-->1], output: [2-->3] in the following graph ($\color{red}\text{red}$ = inside boundary, $\color{blue}\text{blue}$ = boundary edge).

flowchart LR
    0["0"] --> 1["1"]
    1 --> 2["2"]
    2 --> 3["3"]
    dangling["dangling"] --> 2

    style 1 stroke:#D50000
    style 2 stroke:#D50000
    style dangling stroke:#D50000
    linkStyle 0 stroke:#2962FF,fill:none
    linkStyle 1 stroke:#D50000,fill:none
    linkStyle 2 stroke:#2962FF,fill:none
    linkStyle 3 stroke:#D50000

Loading

The code started the toposort using only 1 as initial node to avoid the full graph traversal, and as result never processed 2.

The solution here is to add dangling to the initial set too, but doing so eagerly requires either computing the subgraph first (O(n) space) or doing a full graph traversal.

In my last commit I added the full graph traversal option, but only when we realize we are stuck.
I think avoiding the O(n) memory overhead is nice since it lets the user pass their pre-computed subgraphs if they have them, but if you think that's not needed we can compute the subgraph instead.

(there's a new test case in boundary.rs for this)

[aborgna-q]

Uhm, giving it a bit more though I think you have a point in that I should be doing a better job at filtering nodes/edges not in the boundary-induced subgraph.

In this case:

flowchart LR
    0["0"] --> 1["1"]
    2["3"] --> 3["4"]
    1 --> n1["2 (outside boundary)"]
    n1 --> 2

    style 1 stroke:#D50000
    style 2 stroke:#D50000
    linkStyle 0 stroke:#2962FF,fill:none
    linkStyle 1 stroke:#2962FF,fill:none
    linkStyle 2 stroke:#2962FF
    linkStyle 3 stroke:#2962FF,fill:none

Loading

Where input boundary ports = [0->1, 2->3] and output boundary ports = [1->2, 3->4], There is no order dependency between both sections of the boundary but the current algorithm will detect one from traversing 2.

It should be fixable using toposort's port/node filters, and probably computing the induced subgraph on-the-fly if needed if there are dangling nodes.

[aborgna-q]

Fixed. I moved collect_nodes from subgraph.rs to a private Boundary method and added a test for this.

I'm afraid this PR is becoming a bit noisy :/

[lmondada]

Very nice!