feat(hugr-passes): Add `force_order` pass. #1285

doug-q · 2024-07-09T13:28:22Z

codecov · 2024-07-09T13:38:20Z

Codecov Report

Attention: Patch coverage is 97.19101% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.17%. Comparing base (9b04097) to head (41296c0).
Report is 3 commits behind head on main.

Files	Patch %	Lines
hugr-passes/src/force_order.rs	97.19%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1285      +/-   ##
==========================================
+ Coverage   87.04%   87.17%   +0.12%     
==========================================
  Files         103      104       +1     
  Lines       19243    19421     +178     
  Branches    17073    17251     +178     
==========================================
+ Hits        16750    16930     +180     
+ Misses       1716     1712       -4     
- Partials      777      779       +2

Flag	Coverage Δ
rust	`86.64% <97.19%> (+0.15%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

aborgna-q

💥 Nice.
There may just be some problem with newly discovered nodes having higher rank than existing ones.

There's also a solution that doesn't require computing dominators.
After popping from tovisit, if there's other nodes left in in tovisit keep the popped node around and add an order edge from it to the next visited node.
That would ensure that there's a unique topological order.

aborgna-q · 2024-07-09T14:31:29Z

hugr-passes/src/force_order.rs

+/// All dataflow parents which are transitive children of `root`, including
+/// `root` itself, will be ordered.


It'd be better to talk about the dataflow regions being ordered, rather than parent nodes.

aborgna-q · 2024-07-09T14:38:55Z

hugr-passes/src/force_order.rs

+            // we can only add an order edge if the two ops support it
+            let (n1_ot, n2_ot) = (hugr.get_optype(n1), hugr.get_optype(n2));
+            let expected_edge_kind = Some(hugr_core::types::EdgeKind::StateOrder);
+            if n1_ot.other_port_kind(Direction::Outgoing) != expected_edge_kind
+                || n2_ot.other_port_kind(Direction::Incoming) != expected_edge_kind
+            {
+                continue;
+            }


We may want to delay the edge then.

If n1_ot supports state order edges, keep it around until we find a target node which also supports them and add the edge.

I think there are no node types that support state order edges in only one direction? Though it may be incorrect to assume that...

Good point. I can assert for the weird case you describe.

hugr-passes/src/force_order.rs

aborgna-q · 2024-07-09T14:53:59Z

hugr-passes/src/force_order.rs

+                })
+                .collect_vec();
+
+            self.extend(new_nodes);


There is no guarantee that the new nodes have a lower rank than the existing ones in self.tovisit.
We should use an ordered BTreeSet instead, so they can be added to the right position.

Ah right this is much better. I think I prefer BinaryHeap here, since we don't need lookup. Do you have a test case in mind that would tickle this?

Just [0, 1, 2, 3] in your example :)

#[test] fn test_force_order_3() { let (mut hugr, [v0, v1, v2, v3]) = test_hugr(); let rank_map = [(v0, 0), (v1, 1), (v2, 2), (v3, 3)].into_iter().collect(); let topo_sort = force_order_test_impl(&mut hugr, rank_map); assert_eq!(vec![v0, v1, v2, v3], topo_sort); }

aborgna-q · 2024-07-09T15:00:52Z

It'd be nice to do some benchmarks at some point, but we're lacking test hugrs to benchmark on :/

Co-authored-by: Agustín Borgna <[email protected]>

aborgna-q · 2024-07-09T16:10:15Z

Btw, a more flexible API could be to offer a force_order_by_key where instead of a rank you take a

key: impl Fn(Node) -> (impl Ord)

which is just a generalization of rank.

doug-q · 2024-07-10T06:27:01Z

@aborgna-q I've made most of the algorithmic changes you suggested. I do not like the idea of mutating the hugr from within visit, this seems dangerous and unnecessary. Note that the iteration order is deterministic because each node has a unique sort position to_visit.

Instead, I have observed that all DataflowChild ops have state order edges. See what you think.
EDIT: note that ScopedDefns are DataflowChild. Somewhat surprisingly, they also allow order edges.

I've not edited docs yet, will ask for re-review when I'm done.

doug-q · 2024-07-10T06:43:24Z

Note I've added the passes to lib.rs, lmk if you don't like it.

aborgna-q

👍

Just some minor code suggestions

hugr-passes/src/force_order.rs

Co-authored-by: Agustín Borgna <[email protected]>

## 🤖 New release * `hugr`: 0.6.1 -> 0.7.0 * `hugr-core`: 0.3.1 -> 0.4.0 * `hugr-passes`: 0.3.0 -> 0.4.0 * `hugr-cli`: 0.1.2 -> 0.1.3 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.7.0 (2024-07-10) ### Bug Fixes - Bring back input_extensions serialized field in rust NodeSer ([#1275](#1275)) - [**breaking**] `ops::Module` now empty struct rather than unit struct ([#1271](#1271)) ### Features - Add `force_order` pass. ([#1285](#1285)) - [**breaking**] `MakeOpDef` has new `extension` method. ([#1266](#1266)) ### Refactor - [**breaking**] Remove `Value::Tuple` ([#1255](#1255)) - [**breaking**] Rename `HugrView` function type methods + simplify behaviour ([#1265](#1265)) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) ### Testing - Add a test for [#1257](#1257) ([#1260](#1260)) </blockquote> ## `hugr-core` <blockquote> ## 0.4.0 (2024-07-10) ### Bug Fixes - Bring back input_extensions serialized field in rust NodeSer ([#1275](#1275)) - [**breaking**] `ops::Module` now empty struct rather than unit struct ([#1271](#1271)) ### Features - [**breaking**] `MakeOpDef` has new `extension` method. ([#1266](#1266)) ### Refactor - [**breaking**] Remove `Value::Tuple` ([#1255](#1255)) - [**breaking**] Rename `HugrView` function type methods + simplify behaviour ([#1265](#1265)) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) ### Testing - Add a test for [#1257](#1257) ([#1260](#1260)) </blockquote> ## `hugr-passes` <blockquote> ## 0.4.0 (2024-07-10) ### Features - Add `force_order` pass. ([#1285](#1285)) ### Refactor - [**breaking**] Remove `Value::Tuple` ([#1255](#1255)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.3 (2024-07-10) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).

feat(hugr-passes): Add force_order pass.

b2f5fbb

doug-q requested a review from a team as a code owner July 9, 2024 13:28

doug-q requested a review from aborgna-q July 9, 2024 13:28

tidy up

7838253

doug-q force-pushed the doug/force-order branch from 4588548 to 7838253 Compare July 9, 2024 13:42

aborgna-q reviewed Jul 9, 2024

View reviewed changes

Update hugr-passes/src/force_order.rs

77ac3b3

Co-authored-by: Agustín Borgna <[email protected]>

doug-q added 2 commits July 10, 2024 07:35

make algorithmic changes as suggested in review

90ac0da

edit docs, add force_order_by_key, export functions from lib.rs

34e363c

doug-q requested a review from aborgna-q July 10, 2024 06:43

aborgna-q approved these changes Jul 10, 2024

View reviewed changes

hugr-passes/src/force_order.rs Outdated Show resolved Hide resolved

hugr-passes/src/force_order.rs Outdated Show resolved Hide resolved

hugr-passes/src/force_order.rs Outdated Show resolved Hide resolved

doug-q and others added 3 commits July 10, 2024 12:56

Update hugr-passes/src/force_order.rs

46cffa9

Co-authored-by: Agustín Borgna <[email protected]>

Update hugr-passes/src/force_order.rs

89249d9

Co-authored-by: Agustín Borgna <[email protected]>

remove constraints from struct ForceOrder

41296c0

doug-q added this pull request to the merge queue Jul 10, 2024

Merged via the queue into main with commit 9fe73ed Jul 10, 2024
20 checks passed

doug-q deleted the doug/force-order branch July 10, 2024 12:10

hugrbot mentioned this pull request Jul 10, 2024

chore: release #1248

Merged

hugrbot mentioned this pull request Jul 11, 2024

chore: release #1296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hugr-passes): Add `force_order` pass. #1285

feat(hugr-passes): Add `force_order` pass. #1285

doug-q commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

aborgna-q left a comment

aborgna-q Jul 9, 2024

aborgna-q Jul 9, 2024

doug-q Jul 9, 2024

aborgna-q Jul 9, 2024

doug-q Jul 9, 2024

aborgna-q Jul 9, 2024

aborgna-q commented Jul 9, 2024 •

edited

Loading

aborgna-q commented Jul 9, 2024

doug-q commented Jul 10, 2024 •

edited

Loading

doug-q commented Jul 10, 2024

aborgna-q left a comment

		/// All dataflow parents which are transitive children of `root`, including
		/// `root` itself, will be ordered.

feat(hugr-passes): Add force_order pass. #1285

feat(hugr-passes): Add force_order pass. #1285

Conversation

doug-q commented Jul 9, 2024 • edited Loading

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

aborgna-q left a comment

Choose a reason for hiding this comment

aborgna-q Jul 9, 2024

Choose a reason for hiding this comment

aborgna-q Jul 9, 2024

Choose a reason for hiding this comment

doug-q Jul 9, 2024

Choose a reason for hiding this comment

aborgna-q Jul 9, 2024

Choose a reason for hiding this comment

doug-q Jul 9, 2024

Choose a reason for hiding this comment

aborgna-q Jul 9, 2024

Choose a reason for hiding this comment

aborgna-q commented Jul 9, 2024 • edited Loading

aborgna-q commented Jul 9, 2024

doug-q commented Jul 10, 2024 • edited Loading

doug-q commented Jul 10, 2024

aborgna-q left a comment

Choose a reason for hiding this comment

feat(hugr-passes): Add `force_order` pass. #1285

feat(hugr-passes): Add `force_order` pass. #1285

doug-q commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

aborgna-q commented Jul 9, 2024 •

edited

Loading

doug-q commented Jul 10, 2024 •

edited

Loading