-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply phases using frame_rotation
in QM
#1116
base: interpolated
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## interpolated #1116 +/- ##
================================================
+ Coverage 50.71% 50.73% +0.01%
================================================
Files 63 63
Lines 2918 2917 -1
================================================
Hits 1480 1480
+ Misses 1438 1437 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand why the results are different in the two cases (before and after this PR).
However, the current approach (after this PR) may not be that bad, since it would deduplicate waveforms that are different just because of the relative_phase
, saving memory and networking.
Moreover, that second note is talking about an error of ~1e-5
, which may be relevant at some point, but individually would be compensated by the calibration, and the accumulated one could be counteracted exactly in the way they are describing (which is adding some complexity to the program generation, but if it's efficient in other respects we could accept it - and with our current experiments we should not be that sensitive...).
@@ -21,7 +21,7 @@ def _delay(pulse: Delay, element: str, parameters: Parameters): | |||
duration = max(int(pulse.duration) // 4 + 1, 4) | |||
qua.wait(duration, element) | |||
elif parameters.interpolated: | |||
duration = parameters.duration + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the +1
for?
@@ -73,7 +73,7 @@ def _play( | |||
_play_single_waveform(op, element, parameters, acquisition) | |||
|
|||
if parameters.phase is not None: | |||
qua.reset_frame(element) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the phase was already accounted for in the waveform, why were you resetting the frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, if a frame reset was needed, it could have always been needed, even when the parameters.phase is None
. Since None
should be a relative zero, I guess (in Pulse.relative_phase
we actually use 0.0
as default, and never None
- not sure where the None
is generated).
If instead is not needed, because you're somehow accumulating the relative phases, then it may be never needed.
I'm trying to understand the whole flow, so, I'm still at the level of collecting hints to reconstruct the full picture...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the phase was already accounted for in the waveform, why were you resetting the frame?
Because we were still using frame_rotation_2pi
when sweeping phases, and in that case we may need to reset_frame
. For example doing the following with sweeper:
for theta in sweeper.values:
RX(0, theta=theta)
RX(0, theta=0)
there will be a frame rotation to apply the first gate and we need to reset the frame before applying the second.
Maybe, if a frame reset was needed, it could have always been needed, even when the
parameters.phase is None
.
That's true, I think you can add reset_frame
in the program even without doing any rotation and it should have no effect. However, I think reset_frame
, if not used properly, will screw up VirtualZ
"pulses", that's why I completely removed it from this PR. For example in
RZ(0, theta=np.pi / 4)
RX(0, theta=np.pi / 2)
RX(0, theta=0)
the first np.pi/4
is a virtual-Z and should be maintained throughout the circuit, so if the second RX
causes a reset_frame
in the driver, it won't be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's exactly what I had in mind. The only way I see to make reset_frame
work should be the one described in #1116 (comment), i.e.
- preserving a counter of the accumulated virtual phase in parallel
- restoring the virtual phase after every
reset_frame
However, for the individual RX
it seems simpler to just rotate forward before the gate, and backward right after the play()
instruction (which I expect is what you're now doing). This would accumulate a negligible error on the individual instruction, and we only have to pay attention for long sequences, applying a reset strategy.
I haven't understood that myself yet, that's why it's draft. I just switched to the original approach as I was debugging the CHSH and since I managed to make it work I opened this PR mainly as support of qiboteam/qibocal#1049. The idea is that the qibocal PR should be good as it is and we can decide internally in the driver how to do the phases.
Indeed, however this may not happen in practice right now, because the deduplication is handled by using
I am not sure if this can be solved in calibration. For example, take a single qubit and apply a sequence of random unitaries/U3s. In single qubit calibration we never deal with phases, we only calibrate an RX (or RX90) with zero phase. Then the sequence of random unitaries is compiled to RX90s with random phases, so in principle if there are many gates, this error should affect if we never use |
I'm also considering hashing the generated samples for deduplication, instead of hashing the parameters.
Ok, my point was that we don't need this to make the individual gate optimal. Then, I agree we should still place |
In 0.2 pulse phases was applied in the uploaded waveform samples. Here I am switching this back to the approach used in 0.1, based on
frame_rotation
, because this is how I managed to make the CHSH routine work.I am not sure which solution of the two is better. The one implemented here may suffer from the second note in https://docs.quantum-machines.co/latest/docs/API_references/qua/dsl_main/?h=frame_#qm.qua._dsl.frame_rotation_2pi, so I am leaving it as draft for now.