Gridsynth nit-picks and gate sequence normalisation#1376
Gridsynth nit-picks and gate sequence normalisation#1376PabloAndresCQ merged 5 commits intokc/gridsynth_passfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## kc/gridsynth_pass #1376 +/- ##
=====================================================
- Coverage 79.95% 79.90% -0.06%
=====================================================
Files 159 159
Lines 20851 20858 +7
Branches 19873 19868 -5
=====================================================
- Hits 16672 16667 -5
- Misses 3192 3204 +12
Partials 987 987
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:
|
PabloAndresCQ
requested review from
aborgna-q,
cqc-alec,
doug-q and
ss2165
as code owners
|
Looks to be lots of unrelated changes from merges in this PR? |
|
Oh wow, sorry, something went wrong when I merged And apologies, I didn't mean to request review from anyone other than Chloe, that seems to have been done automatically :/ |
PabloAndresCQ
force-pushed
the
pam/gridsynth_improvements
branch
from
02d64d7 to
5275d23
Compare
| // Remove the W i.e. the exp(i*pi/4) global phases | ||
| let gate_sequence = gates.replacen("W", "", gates.len()); | ||
| // Add the nodes of the gridsynth sequence into the HUGR | ||
| let gridsynth_nodes: Vec<Node> = gate_sequence |
There was a problem hiding this comment.
Nice one! I had a TODO to rewrite this so thanks for taking care of it :)
| while !normal_form_reached { | ||
| // Not the most efficient, but it's easiest to reach the normal form by doing | ||
| // string rewrites. | ||
| // TODO: Can be done with Regex, preferably by providing all LHS to the Regex |
There was a problem hiding this comment.
Just wondering, do you have any idea of the efficiency of using regex vs these replacements?
There was a problem hiding this comment.
Not really, other than it's usually preferred. The two main issues with performance of the current approach are:
- It is not changing the Strings in place, but creating multiple of them. As far as I can tell, Regex doesn't help with that. According to the docs: "The only methods that allocate new strings are the string replacement methods. All other methods (searching and splitting) return borrowed references into the haystack given."
- The other thing is that we scan the String once per iteration and per LHS of a rewrite rule. Ideally, we would find all matches of the same scan and replace them all at once. This can probably be done with Regex' replace_all since it looks like the
Replacercan be a closure|&Captures| -> String(see examples in thereplacedocs).
After staring at the docs of Regex for 30min, I decided it was not worth doing it (I couldn't figure out how to do it) until we've got some evidence that we need better performance here.
3 participants
Merging into the open draft PR #1346 with some improvements