-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: extract_hugr
not removing root node ports
#1239
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1239 +/- ##
==========================================
+ Coverage 86.97% 87.00% +0.02%
==========================================
Files 100 100
Lines 18998 19000 +2
Branches 17013 17015 +2
==========================================
+ Hits 16524 16530 +6
+ Misses 1696 1693 -3
+ Partials 778 777 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
hugr-core/src/hugr/views/sibling.rs
Outdated
|
||
let region: SiblingGraph = SiblingGraph::try_new(&hugr, def)?; | ||
let region: DescendantsGraph = DescendantsGraph::try_new(&hugr, def)?; |
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.
Why are you changing to a DescendentsGraph? I think his test is supposed to be testing functionality of SiblingGraph.
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.
The test hugr contains an inner DFG, so extracting just the SiblingGraph produced an invalid DFG node without children -.-
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.
I think a good solution is to change it to extract the dfg, and assert that the root of the new hugr has no ports
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.
I guess I should have moved this test to descendants.rs
. I'll do that, and add a flat case here for SiblingGraph
@@ -512,6 +512,7 @@ pub trait ExtractHugr: HugrView + Sized { | |||
let old_root = hugr.root(); | |||
let new_root = hugr.insert_from_view(old_root, &self).new_root; | |||
hugr.set_root(new_root); | |||
hugr.set_num_ports(new_root, 0, 0); |
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.
Shouldn't we add a test? make_module_hgr
seems to provide a suitable case, the inner DFG (which has ports) should be extracted.
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.
That's what the descendant graph test is doing. Only SiblingGraph
and DescendantGraph
run this code on extraction.
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.
The test you changed:
- extracts a node that has no ports anyway
- does not assert that the extracted Hugr has no ports
I think this is technically a breaking change? Or would anyone who was using |
Yep, this doesn't change the behaviour except for the actual |
6d6f29e
to
4635273
Compare
## 🤖 New release * `hugr`: 0.5.1 -> 0.6.0 * `hugr-core`: 0.2.0 -> 0.3.0 * `hugr-passes`: 0.2.0 -> 0.3.0 * `hugr-cli`: 0.1.1 -> 0.1.2 <details><summary><i><b>Changelog</b></i></summary><p> ## `hugr` <blockquote> ## 0.6.0 (2024-06-28) ### Bug Fixes - SimpleReplacement panic on multiports ([#1191](#1191)) - Add some validation for const nodes ([#1222](#1222)) - Cfg not validating entry/exit types ([#1229](#1229)) - `extract_hugr` not removing root node ports ([#1239](#1239)) ### Documentation - Fix documentation of `ValidationError::ConstTypeError` ([#1227](#1227)) ### Features - CircuitBuilder::add_constant ([#1168](#1168)) - [**breaking**] Make the rewrite errors more useful ([#1174](#1174)) - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - [**breaking**] Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop ([#1195](#1195)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) ### Refactor - [**breaking**] FunctionBuilder takes impl Into<PolyFuncType> ([#1220](#1220)) - [**breaking**] Remove NodeType and input_extensions ([#1183](#1183)) </blockquote> ## `hugr-core` <blockquote> ## 0.3.0 (2024-06-28) ### Bug Fixes - SimpleReplacement panic on multiports ([#1191](#1191)) - Add some validation for const nodes ([#1222](#1222)) - Cfg not validating entry/exit types ([#1229](#1229)) - `extract_hugr` not removing root node ports ([#1239](#1239)) ### Documentation - Fix documentation of `ValidationError::ConstTypeError` ([#1227](#1227)) ### Features - CircuitBuilder::add_constant ([#1168](#1168)) - [**breaking**] Make the rewrite errors more useful ([#1174](#1174)) - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - [**breaking**] Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop ([#1195](#1195)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) ### Refactor - [**breaking**] Remove NodeType and input_extensions ([#1183](#1183)) - [**breaking**] FunctionBuilder takes impl Into<PolyFuncType> ([#1220](#1220)) </blockquote> ## `hugr-passes` <blockquote> ## 0.3.0 (2024-06-28) ### Features - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.1 (2024-06-07) ### Features - Reexport `clap::Parser` and `clap_verbosity_flag::Level` from hugr_cli ([#1146](#1146)) ### Refactor - Move binary to hugr-cli ([#1134](#1134)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Douglas Wilson <douglas.wilson@quantinuum.com>
Closes #1238