refactor: add HugrView::scheduling_graph, deprecate region_portgraph#3015
Conversation
1356b19 to
6db3845
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3015 +/- ##
==========================================
- Coverage 81.32% 81.07% -0.26%
==========================================
Files 240 242 +2
Lines 45407 45183 -224
Branches 39175 38951 -224
==========================================
- Hits 36929 36633 -296
- Misses 6488 6585 +97
+ Partials 1990 1965 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This reverts commit 04178e4.
6db3845 to
521319f
Compare
Merging this PR will degrade performance by 23.4%
Performance Changes
Comparing Footnotes
|
| }; | ||
| use std::collections::BTreeSet; | ||
|
|
||
| use portgraph::{SecondaryMap, UnmanagedDenseMap}; |
There was a problem hiding this comment.
This file is basically a straight copy of TopoConvexChecker from portgraph then slightly reparametrized
| @@ -0,0 +1,778 @@ | |||
| use std::collections::BTreeSet; | |||
There was a problem hiding this comment.
Tests moved out of sibling_subgraph.rs
|
|
||
| impl<T: LinkView> pv::GraphBase for SynEdgeWrapper<T> { | ||
| type NodeId = NIdx<T::NodeIndexBase>; | ||
| type EdgeId = ( |
There was a problem hiding this comment.
Note this is significantly more informative than portgraph's petgraph-traits, but perhaps it shouldn't be
|
|
||
| impl<T: LinkView> pv::NodeIndexable for SynEdgeWrapper<T> { | ||
| fn node_bound(&self) -> usize { | ||
| // ALAN copied from petgraph; but are `NodeIndex`es always dense? |
There was a problem hiding this comment.
I was a bit worried here??
There was a problem hiding this comment.
Ouch, that looks wrong.
It should be self.region_view.node_capacity() instead, since node indices do not get compacted after a removal.
| /// but at least this distinguishes synthetic edges from original edges. | ||
| /// | ||
| /// [FlatRegion]: portgraph::view::FlatRegion | ||
| type EdgeWeight = MaybeSynEdge<(PortOffset<T::PortOffsetBase>, PortOffset<T::PortOffsetBase>)>; |
There was a problem hiding this comment.
Again, portgraph's petgraph-traits does not provide any edge info, not even just the port offsets
| /// Access to the graph, sufficient to allow [pv::Topo] | ||
| pub fn petgraph( | ||
| &self, | ||
| ) -> impl pv::NodeCount |
There was a problem hiding this comment.
this type is part of the hiding, and quite deliberate. It could be much more detailed (NodeId/EdgeId types etc.)
| /// Skip convexity check. | ||
| SkipConvexity, | ||
| mod hidden { | ||
| #![expect(deprecated)] // Remove enum along with TopoConvexChecker |
There was a problem hiding this comment.
This mod hidden is so we can do this to capture the deprecation warning from #[derive(Default)]
There was a problem hiding this comment.
You can avoid the module by using allow instead of expect. (Seems like a bug, but this works)
#[deprecated]
#[allow(deprecated)]
#[derive(Default)]
pub enum E {
#[default]
A
}There was a problem hiding this comment.
Hah! Yes, that is odd, I had tried expect. Thanks!
| /// particularly when many checks must be performed. | ||
| pub type LineConvexChecker<'g, Base> = | ||
| ConvexChecker<'g, Base, portgraph::algorithms::LineConvexChecker<CheckerRegion<'g, Base>>>; | ||
| pub type LineConvexChecker<'g, Base> = PortgraphCheckerWithNodes< |
There was a problem hiding this comment.
So I've put some effort into trying to keep this alive, on the grounds that it's potentially much more efficient than TopoConvexChecker. I think it works only for graphs with no container nodes, in which case there will be no syn edges, so it'll still be as ok as it ever was.
But, we could also drop it and plan to reimplement line checking somewhere else if we want it, possibly based off Quantinuum/tket2#1289 somehow
There was a problem hiding this comment.
Nice! If it works with your changes I would keep it.
| outputs: &OutgoingPorts<H::Node>, | ||
| function_calls: &IncomingPorts<H::Node>, | ||
| ) -> Result<Vec<H::Node>, InvalidSubgraph<H::Node>> { | ||
| // Compute the nodes inside the boundary ignoring synthetic edges - |
There was a problem hiding this comment.
This is key, can you check that you agree?
There was a problem hiding this comment.
The logic looks good to me
| // that would lead to needing those synthetic edges) then the subgraph is invalid | ||
| // (nodes would not share the same parent)! | ||
| let node_indices = make_pg_subgraph::<H>( | ||
| self.checker.graph().region_view.clone(), |
There was a problem hiding this comment.
So I guess this is the same backdoor as portgraph_no_syn_edges, and a slight shame I can't use the same method, but here I think it is the right thing to do
aborgna-q
left a comment
There was a problem hiding this comment.
LGTM asides from the node bound bug.
| /// Skip convexity check. | ||
| SkipConvexity, | ||
| mod hidden { | ||
| #![expect(deprecated)] // Remove enum along with TopoConvexChecker |
There was a problem hiding this comment.
You can avoid the module by using allow instead of expect. (Seems like a bug, but this works)
#[deprecated]
#[allow(deprecated)]
#[derive(Default)]
pub enum E {
#[default]
A
}| /// particularly when many checks must be performed. | ||
| pub type LineConvexChecker<'g, Base> = | ||
| ConvexChecker<'g, Base, portgraph::algorithms::LineConvexChecker<CheckerRegion<'g, Base>>>; | ||
| pub type LineConvexChecker<'g, Base> = PortgraphCheckerWithNodes< |
There was a problem hiding this comment.
Nice! If it works with your changes I would keep it.
| outputs: &OutgoingPorts<H::Node>, | ||
| function_calls: &IncomingPorts<H::Node>, | ||
| ) -> Result<Vec<H::Node>, InvalidSubgraph<H::Node>> { | ||
| // Compute the nodes inside the boundary ignoring synthetic edges - |
There was a problem hiding this comment.
The logic looks good to me
|
|
||
| /// A view of a flat region, including ordering constraints from nonlocal edges, | ||
| /// suitable for use with petgraph algorithms. | ||
| fn scheduling_graph(&self, parent: Self::Node) -> SchedulingGraph<'_, Self> { |
There was a problem hiding this comment.
I like scheduling_graph, it conveys well the causal order meaning.
|
|
||
| impl<T: LinkView> pv::NodeIndexable for SynEdgeWrapper<T> { | ||
| fn node_bound(&self) -> usize { | ||
| // ALAN copied from petgraph; but are `NodeIndex`es always dense? |
There was a problem hiding this comment.
Ouch, that looks wrong.
It should be self.region_view.node_capacity() instead, since node indices do not get compacted after a removal.
This paves the way for #2946, but does not solve. For that, we will need to add "fake" edges: for each nonlocal edge, from the source to the sibling that is ancestor of the target. (Currently stored in Hugr as order edges but these will be removed). However, it will not be possible to add these fake edges to the
region_portgraph: being a portgraph (view of hugr), there is no port for the fake edges.Instead this PR introduces a new method
scheduling_graphreturning a new SchedulingGraph structure: a generic petgraph, which augments a view of the Hugr with an explicit list of extra edges. In this PR, said list is empty, as the Hugr still contains the order edges necessary to ensure topological orderings are sound. #2951 removes the order edges from Hugr and generates a list of those required as part of the SchedulingGraph.This PR includes moving
sibling_subgraph.rstests into a separatetests.rs, and also copies theTopoConvexCheckerfrom portgraph (reparametrized to work on a petgraph).Add some tests demonstrating existing behaviour of SiblingSubgraph and
scheduling_graphwith order edges, these are then updated in #2951 to show the (i.e. breaking) changes in behaviour.