Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1346 +/- ##
==========================================
+ Coverage 79.74% 79.97% +0.23%
==========================================
Files 158 159 +1
Lines 20509 20869 +360
Branches 19542 19868 +326
==========================================
+ Hits 16355 16691 +336
- Misses 3173 3192 +19
- Partials 981 986 +5
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:
|
|
I'll have a look through the PR and play with it a bit, then mark it as ready for review when satisfied on my end. @acl-cqc, I've seen that the Cargo.lock has a bunch of new packages pulled by the |
| # is not run first then Gridsynth is likely to fail. Maybe issue the warning if | ||
| # the option to run NormalizeGuppy is set to False. The option would be specified | ||
| # as a field of the dataclass (would also need to add @dataclass decorator) | ||
| # like for NormalizeGuppy above |
There was a problem hiding this comment.
I think it's fine that NormalizeGuppy is always run at the start of the pass, as it is a pre-requesite. Is it a problem if NormalizeGuppy is run more than once? I expect it should leave the HUGR unchanged (I haven't checked).
| # like for NormalizeGuppy above | ||
| def run(self, hugr: Hugr, *, inplace: bool = True) -> PassResult: | ||
| # inplace option does nothing for now but I retain for consistency of | ||
| # API with other passes |
There was a problem hiding this comment.
I expect we don't need to do anything special with inplace here, is it not enough to delegate to implement_pass_run as done below?
There was a problem hiding this comment.
Yeah, implement_pass_run takes care of dealing with the missing case.
| fn add_references(&mut self, node: Node, increment: usize) { | ||
| // if reference not in references add it with the default value 1, else increment count | ||
| let count = self.references.entry(node.index()).or_insert(1); | ||
| *count += increment; |
There was a problem hiding this comment.
Considering that you are incremeting by increment later, shouldn't you use default value 0 when the entry does not exist?
There was a problem hiding this comment.
Indeed, I have looked at the HUGRs that come out of the pass, and the constant nodes are not garbage collected. Likely due to this.
| } | ||
|
|
||
| /// If there are no references remaining to const_node, remove it and the nodes leading to it | ||
| fn collect(&mut self, hugr: &mut Hugr, const_node: Node) { |
There was a problem hiding this comment.
Unfortunate name: collect in Rust is used as a method that converts an iterable to some data structure. Sure, here you meant to say "request garbage collection", but probably a different name should be used. Maybe clear_garbage.
| // value: reference counter for that node | ||
| path: HashMap<usize, Vec<Node>>, // key: node index (of Const node containing angle), | ||
| // value: the nodes leading up to the constant node and the constant | ||
| // node itself |
There was a problem hiding this comment.
Since there is a single Vec<Node> per Const node key in path, it implies that there is an assumption that this constant node is used in a single place (otherwise there'd be multiple paths!). As such, it follows that the values of references should always be 1.
I don't see the point of using a GarbageCollector unless we admit the same Const node being used in multiple places in the HUGR.
Bumped tket-c-api dependency to avoid boost 1.90 dependency mismatch
Merging into the open draft PR #1346 with some improvements
PabloAndresCQ
left a comment
PabloAndresCQ
left a comment
There was a problem hiding this comment.
I found a case that broke in the current state of the gridsynth PR. If we have something like this:
@guppy
def foo():
// Interesting quantum program here
@guppy
def bar():
foo()
If you run Gridsynth on foo.compile() it works, but if you run it on bar.foo() it doesn't. It complains with a "index out of bounds: len 0 but index 0" coming from the Rust code. A workaround is to call inline_acyclic to inline every function. I do not know what's actually causing this, it'd be good to solve it with a less drastic approach.
Also, this should be a simple unit test to include.
3 participants
Adding a pass that applies an open source rust implementation of the gridsynth algorithm to all Rz gates in a HUGR. Gridsynth decomposes Rz gates into the Clifford + T basis. Python bindings to enable users to directly modify Guppy-generated HUGRs in Python are included.
As part of the pass, the constant node used to load the angle inputted to the Rz and any intermediary nodes are cleaned up using a garbage collection strategy.
A related demo for the pass is available in the private https://github.com/Quantinuum/gridsynth_guppy_demo.git repo. The demo also functions as a series of manual integration tests for using the pass on Guppy generated HUGRs in Python, albeit using a Jupyter notebook.