feat: allow negative controlled global phase gate #216
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 48 48
Lines 3802 3810 +8
Branches 930 932 +2
=========================================
+ Hits 3802 3810 +8 ☔ View full report in Codecov by Sentry. |
jcjaskula-aws
changed the title
| @@ -447,6 +447,7 @@ def _(self, node: QuantumGate) -> None: | |||
|
|
|||
| @visit.register | |||
| def _(self, node: QuantumPhase) -> None: | |||
| self._uses_advanced_language_features = True | |||
There was a problem hiding this comment.
Hm, this was previously not counted here because the gates were defined in terms oq3 built-in gates so there would be false positives. Might be safe to have this now that the gates get simulated directly.
There was a problem hiding this comment.
Happy to remove it. I did this to limit the number of circuit with U/gphase sent to the service but I think the number of circuits built with from_ir will be marginal anyway.
| qubit q; | ||
| h q; | ||
| negctrl @ gphase(π) q; | ||
| h q; |
There was a problem hiding this comment.
would this test be simpler to grok if the circuit was just x q; negctrl @ gphase(π) q;? I think this test would be extra compelling if we could also assert that it's equal to an equivalent circuit that uses x and ctrl @ gphase
There was a problem hiding this comment.
maybe h is better since it's more convincing we're not just getting lucky with the transformation since we're in a basis state
There was a problem hiding this comment.
H is better as ctrl @ gphase has a visible effect on the X basis. I have added a much more complete test.
|
I wouldn't label this as a fix, as the previous implementation fit the spec. This is an extension of functionality |
| [Identifier("π"), IntegerLiteral(0), Identifier("π")], | ||
| controlled_phase.qubits, | ||
| ) | ||
| return [X, ctrl_phaseshift, X] |
There was a problem hiding this comment.
I'm confused how this is being handled without any changes to inline_gate_def_body
There was a problem hiding this comment.
me too. Reworked the functions that needed a change.
jcjaskula-aws
changed the title
| @@ -54,17 +54,16 @@ def is_controlled(phase: QuantumPhase) -> bool: | |||
| return False | |||
|
|
|||
|
|
|||
| def convert_phase_to_gate(controlled_phase: QuantumPhase) -> QuantumGate: | |||
| def convert_phase_to_gate(controlled_phase: QuantumPhase) -> Union[QuantumGate, list[QuantumGate]]: | |||
There was a problem hiding this comment.
This approach increases complexity, particularly in inline_gate_def_body which has a lot of logic embedded already. I wonder if we shouldn't be handling "controlled phase instructions" logic during interpretation, but rather in ProgramContext.add_phase_instruction. I guess it comes down to whether controlled phases are considered part of the spec, or up to implementation (which now we do a mix of both)
There was a problem hiding this comment.
I think it is a nice point you are raising. It is problably ok to defer later to the context construction. The BDK should anyway have the logic to handle it if necessary.
Issue #, if available:
Negative controlled gphase are executable via a simple translation.
Description of changes:
We provide the translation for negctrl @ gphase
Testing done:
Added unit tests.
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.