feat!: do not require an Order edge for each nonlocal Ext edge; compute on demand in scheduling_graph()#2951
feat!: do not require an Order edge for each nonlocal Ext edge; compute on demand in scheduling_graph()#2951
Ext edge; compute on demand in scheduling_graph()#2951Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2951 +/- ##
==========================================
+ Coverage 81.07% 81.15% +0.07%
==========================================
Files 242 240 -2
Lines 45183 44954 -229
Branches 38951 38724 -227
==========================================
- Hits 36633 36483 -150
+ Misses 6585 6497 -88
- Partials 1965 1974 +9
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
Ext edge; order_graph computes on demandExt edge; compute on demand in scheduling_graph()
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
| /// The API is designed for `hugr-persistent` and allows to implement [HugrView] such that | ||
| /// [HugrInternals::RegionPortgraph] is not tied to the original view but rather an owned | ||
| /// temporary [Hugr]. | ||
| pub fn owned_scheduling_graph<'a, N: HugrNode, V>( |
There was a problem hiding this comment.
This is broadly equivalent to the removed Hugr::into_region_portgraph from internals.rs; it could be an instance method again (into_scheduling_graph) but seems more consistent in this file.
It is definitely the part of the PR that I'm least keen on!! But either we expose more internal details of SchedulingGraph (which I've been trying to avoid in the design, to keep some flexibility to change in the future), and use those details in hugr-persistent; or, we put some hugr-persistent-specific functionality here. This PR chooses the latter route...
(but it doesn't seem that weird a functionality, even if hugr-persistent is the only place we're using it ATM.)
Thoughts welcome!
| respecting the Dataflow edges.** The following operations are used to | ||
| express control flow, i.e. conditional or repeated evaluation. | ||
|
|
||
| **ALAN this might be a place to either say, and respecting the ordering |
There was a problem hiding this comment.
Yes, could be worth saying that descendent nodes may be interleaved with siblings, subject to the ordering constraints imposed by Dataflow edges.
There was a problem hiding this comment.
I have done - I realize that I was actually planning for that to come later along with tweaks to some Hugr code to enact it, but it's not a correction to the spec (it doesn't change anything that was there before AFAICS), merely a clarification; and whilst there is some Hugr code that sort-of-assumes otherwise, I think updating that can be classed as an improvement or optimization (or just possibly bugfix) i.e. in another PR.
So, would you mind having another quick look over the spec 🙏 ? I also explicitly mentioned respecting Order edges too (but not synthetic/nonlocal edges since there is no need to).
|
|
||
| def test_build_inter_graph(snapshot): | ||
| # Possibly a bit redundant now, really we're just testing that we *don't* do | ||
| # anything special anymore, following https://github.com/Quantinuum/hugr/pulls/2951. |
There was a problem hiding this comment.
| # anything special anymore, following https://github.com/Quantinuum/hugr/pulls/2951. | |
| # anything special anymore, following https://github.com/Quantinuum/hugr/pull/2951. |
There was a problem hiding this comment.
I am humbled. I would never have spotted that myself :-)
| respecting the Dataflow edges.** The following operations are used to | ||
| express control flow, i.e. conditional or repeated evaluation. | ||
|
|
||
| **ALAN this might be a place to either say, and respecting the ordering |
There was a problem hiding this comment.
Yes, could be worth saying that descendent nodes may be interleaved with siblings, subject to the ordering constraints imposed by Dataflow edges.
BREAKING CHANGE: Hugrs are no longer guaranteed to have an Order edge representing the ordering constraint from each nonlocal
Extedge; require HugrView impls to provide scheduling_graph; remove deprecated region_portgraph(), as_petgraph(), into_region_portgraph(), sibling_subgraph::{TopoConvexChecker, ValidationMode, SiblingSubgraph::validate(), ConvexChecker}, ValidationError::MissingOrderEdge