-
Notifications
You must be signed in to change notification settings - Fork 7
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
docs: attempt to correct force_order docs #1299
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1299 +/- ##
=======================================
Coverage 87.18% 87.18%
=======================================
Files 107 107
Lines 19538 19538
Branches 17276 17276
=======================================
Hits 17034 17034
Misses 1719 1719
Partials 785 785
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/// deterministic. | ||
/// Nodes are ordered according to the `rank` function but respecting the order | ||
/// required for their dependencies (edges). The algorithm will put nodes of | ||
/// lower rank earlier in their parent whenever (transitive) dependencies allow. |
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.
An alternative would be to error out in the impossible case, rather than let dependencies overrule rank
, i.e. if the rank
said to put n1
before n2
but n1
depends upon n2
. That's a code change rather than just a docs change, tho!
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 is deliberately allowed. I want to give quantum ops a low rank and read-future ops a high rank, and have them reordered as well as allowed. I don't want to have to figure out a legal ranking, that's what the pass is for!
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.
Thank you Alan, this is certainly an improvement.
/// deterministic. | ||
/// Nodes are ordered according to the `rank` function but respecting the order | ||
/// required for their dependencies (edges). The algorithm will put nodes of | ||
/// lower rank earlier in their parent whenever (transitive) dependencies allow. |
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 is deliberately allowed. I want to give quantum ops a low rank and read-future ops a high rank, and have them reordered as well as allowed. I don't want to have to figure out a legal ranking, that's what the pass is for!
## 🤖 New release * `hugr`: 0.7.0 -> 0.8.0 * `hugr-core`: 0.4.0 -> 0.5.0 * `hugr-passes`: 0.4.0 -> 0.5.0 * `hugr-cli`: 0.1.3 -> 0.1.4 <details><summary><i><b>Changelog</b></i></summary><p> ## `hugr` <blockquote> ## 0.8.0 (2024-07-16) ### Bug Fixes - [**breaking**] Force_order failing on Const nodes, add arg to rank. ([#1300](#1300)) - NonConvex error on SiblingSubgraph::from_nodes with multiports ([#1295](#1295)) - [**breaking**] Ops require their own extension ([#1226](#1226)) ### Documentation - Attempt to correct force_order docs ([#1299](#1299)) ### Features - Make `DataflowOpTrait` public ([#1283](#1283)) - Make op members consistently public ([#1274](#1274)) ### Refactor - [**breaking**] Rename builder helpers: ft1->endo_ft, ft2->inout_ft ([#1297](#1297)) </blockquote> ## `hugr-core` <blockquote> ## 0.5.0 (2024-07-16) ### Bug Fixes - NonConvex error on SiblingSubgraph::from_nodes with multiports ([#1295](#1295)) - [**breaking**] Ops require their own extension ([#1226](#1226)) ### Features - Make `DataflowOpTrait` public ([#1283](#1283)) - Make op members consistently public ([#1274](#1274)) ### Refactor - [**breaking**] Rename builder helpers: ft1->endo_ft, ft2->inout_ft ([#1297](#1297)) </blockquote> ## `hugr-passes` <blockquote> ## 0.5.0 (2024-07-16) ### Bug Fixes - [**breaking**] Ops require their own extension ([#1226](#1226)) - [**breaking**] Force_order failing on Const nodes, add arg to rank. ([#1300](#1300)) ### Documentation - Attempt to correct force_order docs ([#1299](#1299)) ### Refactor - [**breaking**] Rename builder helpers: ft1->endo_ft, ft2->inout_ft ([#1297](#1297)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.3 (2024-07-10) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).
I don't think "dominance" is what you want here -
n1
dominatesn2
if all paths from the root ton2
go vian1
, I don't think this is really relevant. (I also found it a little hard to understand what guarantee we did offer given the explicit disclaimer....)Hopefully this is less ambiguous, although I suspect it is somewhat redundant/repetitive, maybe it could be improved further...