fix: Ops require their own extensions #734
Conversation
… use input extensions of Const
| while !new_variables.is_empty() { | ||
| new_variables = new_variables | ||
| .into_iter() | ||
| .filter(|m| seen.insert(*m)) | ||
| .flat_map(|m| self.get_constraints(&m).unwrap()) | ||
| .flat_map(|m| self.get_constraints(&m).unwrap_or(&constraints_for_solved)) |
| @@ -189,6 +193,25 @@ fn test_dataflow_ports_only() { | |||
| ) | |||
| .unwrap(); | |||
| dfg.add_other_wire(not.node(), call.node()).unwrap(); | |||
|
|
|||
| // As temporary workaround for https://github.com/CQCL/hugr/issues/695 | |||
There was a problem hiding this comment.
This is a bit nasty, but should be solved by the two issues linked in due course.
…struct_simple_replacement. Ugh...
acl-cqc
force-pushed
the
fix/ops_require_ext
branch
from
e7c2940 to
89cea89
Compare
| @@ -246,9 +246,6 @@ impl SignatureFunc { | |||
| }; | |||
|
|
|||
| let res = pf.instantiate(args, exts)?; | |||
| // TODO bring this assert back once resource inference is done? | |||
| // https://github.com/CQCL/hugr/issues/388 | |||
| // debug_assert!(res.extension_reqs.contains(def.extension())); | |||
There was a problem hiding this comment.
The assert should stay in, no?
There was a problem hiding this comment.
This is now the inner function, called by the outer which modifies the result to add the necessary extra Extension.
There was a problem hiding this comment.
But, seems we can now just do it here. A lot has changed since this PR started!!
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #734 +/- ##
==========================================
+ Coverage 84.47% 84.61% +0.13%
==========================================
Files 67 67
Lines 13214 13374 +160
Branches 13214 13374 +160
==========================================
+ Hits 11163 11316 +153
Misses 1270 1270
- Partials 781 788 +7 ☔ View full report in Codecov by Sentry. |
|
Discussion with @ss2165 on 5th Jan - agreed Lift nodes like this are too painful. Hoping to switch to a subset constraint (at validation time) - inference algorithm details TBC... |
|
Superceded by #1226 |
The "fix" here is about three lines in op_def.rs. The rest is basically just updating tests to reflect the extra extension requirements, although there is a two-line fix in
infer.rs, see here. I've also done this as a followup to #733, it may be applicable without.WIP: the fixes here to sibling_subgraph do not look great, I have to say. If we agree this PR is indeed the way forward then we should discuss with @lmondada what to do here.
The test updates are straightforward, but massive, and rather painful. I see at least three options:
lifthelper to the builder which just takes wires, and calculates the necessary type-row for them. (Another possibility:with_extension_deltacould have variants that take single Extensions rather than a set.)I guess I'm arguing against doing (1) temporarily and then dropping lift nodes (and removing all those utilities) in the future...
closes #388