Adds global phase to the PhaseShift decomposition
#4657
Conversation
|
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4657 +/- ##
=========================================
Coverage ? 99.65%
=========================================
Files ? 388
Lines ? 35158
Branches ? 0
=========================================
Hits ? 35038
Misses ? 120
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
Just a few small comments, I think it would also be useful to look for decompositions of PhaseShift that don't use the GlobalPhase operator until we have a decomposition for it |
This is a good point, as I think we risk plugin devices failing with this change as they will not know how to handle @obliviateandsurrender - could we merge this PR after the 0.33 release to give us time to evaluate any issues? |
There was a problem hiding this comment.
the previous discussion kinda makes me worried. this is breaking for anything that didn't support PhaseShift but also doesn't support GlobalPhase (idk if any devices do other than default.qubit today). I'm not sure what would change after 0.33 is released, but we might need to work out a better plan for general GlobalPhase support before doing this.
Agreed, I think we can just give a decomposition to the identity. This is wrong, but only as wrong as it was before. |
|
works for me... but what wire will we give to the decomposition 😅 |
I want to say that it should be the wires of |
|
the op has no wires. the argument is only there to not break constructors - Lillian tried to remove it and it made a huge mess for some reason. If you give them and they match all device wires, nice! If they don't match, your circuit will fail. But if you don't give them at all, the op wires are an empty list. So, if you try to decompose to Identity in that case (a common one, who would give wires to a global phase??), we won't know what wires to use. idea: remove the need for wires from Identity as well. |
**Context:** Adds `GlobalPhase` to `PhaseShift` decomposition, which is right now missing and simply expands to `RZ`. **Description of the Change:** Changes the `compute_decomposition` for `PhaseShift` and a few tests using the previous decomposition **Benefits:** We wouldn't miss out on the phase information when decomposing this gate. **Possible Drawbacks:** N/A **Related GitHub Issues:** N/A **Note for reviewers:** If a device does not support `PhaseShift` and _also_ doesn't support `GlobalPhase`, this should not cause any trouble because `GlobalPhase` decomposes to nothing. It won't have the phase information, but it didn't in the past either (just the `RZ`, so it will remain correct up to a phase)
**Context:** Adds `GlobalPhase` to `PhaseShift` decomposition, which is right now missing and simply expands to `RZ`. **Description of the Change:** Changes the `compute_decomposition` for `PhaseShift` and a few tests using the previous decomposition **Benefits:** We wouldn't miss out on the phase information when decomposing this gate. **Possible Drawbacks:** N/A **Related GitHub Issues:** N/A **Note for reviewers:** If a device does not support `PhaseShift` and _also_ doesn't support `GlobalPhase`, this should not cause any trouble because `GlobalPhase` decomposes to nothing. It won't have the phase information, but it didn't in the past either (just the `RZ`, so it will remain correct up to a phase)
Context: Adds
GlobalPhasetoPhaseShiftdecomposition, which is right now missing and simply expands toRZ.Description of the Change: Changes the
compute_decompositionforPhaseShiftand a few tests using the previous decompositionBenefits: We wouldn't miss out on the phase information when decomposing this gate.
Possible Drawbacks: N/A
Related GitHub Issues: N/A
Note for reviewers:
If a device does not support
PhaseShiftand also doesn't supportGlobalPhase, this should not cause any trouble becauseGlobalPhasedecomposes to nothing. It won't have the phase information, but it didn't in the past either (just theRZ, so it will remain correct up to a phase)