Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add NormalizeRXAngle and RXCalibrationBuilder passes #10634
Add NormalizeRXAngle and RXCalibrationBuilder passes #10634
Changes from 6 commits
92344c2
5e23f55
acb5ee9
c681760
79a44d8
2b0664c
fe1fc06
9244800
60fb2c4
87f15df
6d6a059
35e5b93
3011bb9
1ff6c50
e56bb20
aa0270c
12cbeb0
dac2c64
9a5337f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is probably fine, but it makes me a little nervous. I would do more checking to confirm that the default calibration is a single Drag pulse.
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 added two tests in the
supported()
method to check if the default calibration is a single pulse and it's a DRAG pulse.However, I can't decide whether we should allow non-DRAG pulses too or not.
Pros for allowing non-DRAG pulses: Flexibility. This pass simply scales the amplitude of the default calibration, based on preliminary experiments that indicated no need to alter the DRAG coefficient. Since all other types of pulses (Gaussian, square, etc) have the amplitude parameter, it's technically possible to scale the amplitude of any type of pulses.
Cons for allowing non-DRAG pulses: The goal of this pass is to maximize the gate fidelity by decreasing gate time. The gate fidelity might not be optimal with non-DRAG pulses. Therefore, the user should probably try using DRAG pulses first, instead of using this pass.
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 think it would be okay to check for
ScalableSymbolicPulse
and scale theamp
parameter in that. Since this is not part of a preset passmanager and is a somewhat advanced concept, I think it is okay to assume that the user has some understanding of the pulse to be scaled. I was more concerned about a using doing something unusual like using a schedule with two pulses or with a variant ofDrag
and not realizing the schedule was being replaced with a singleDrag
.Also,
isinstance(pulse, Drag)
is going to be deprecated. The recommendation is to useisinstance(pulse, SymoblicPulse) and pulse.pulse_type == "Drag"
becauseDrag
and the other symbolic pulses are being converted into factory functions that produceSymbolicPulse
s instead of classes themselves. If you switch to checking forScalableSymbolicPulse
and scaling theamp
this won't matter.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.
Thank you @wshanks, I'm trying to cover the case where the original
sx
calibration is aScheduleBlock
with multiple types of instructions and multiple types of pulses, as you pointed out.Could you help me building a new ScheduleBlock where everything else is the same but only the amplitude is halved?
I'm having trouble copying the alignment constraints and non-Play type of instructions.
This is an example of an unusually complicated sx schedule:
This is my attempt:
Questions:
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.
When I print out the above
rx_cal
, the pulses seem to have the correct types (ex.Drag
andGaussianSquare
).ScheduleBlock(Play(Drag(duration=100, sigma=40, beta=0.2, amp=0.05, angle=10), DriveChannel(0)), Play(GaussianSquare(duration=130, sigma=30, width=10.0, amp=0.15, angle=30), DriveChannel(0)), name="block25", transform=AlignLeft())
However, I get
Pulse envelope expression is not assigned
errors when I try to draw the schedule.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.
Hmm, we can see what @nkanazawa1989 thinks. One option is not to try to handle such a complicated case, but to choose either to give an error or ignore it. A complicated schedule seems not too likely to come up, and either giving an error or ignoring the gate would avoid the case of assigning the wrong schedule (i.e. assigning a Drag with amplitude based on the first pulse amplitude if the schedule really has multiple instructions).
I think it's pretty tricky to walk through a
ScheduleBlock
and modify the pulses. I think I would not use the builder interface to do it. There is aScheduleBlock.replace
method that you could use like:but that only covers the top level blocks. You would also need to check
isinstance(block, ScheduleBlock)
and recurse into the nestedScheduleBlock
s and replace pulses in them and callreplace(block, new_scheduleblock)
.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 see, I agree that it's reasonable to reject a schedule with multiple pulses.
The current implementation of supported() checks that the sx cal is a single pulse AND it's a Drag.
https://github.com/Qiskit/qiskit/pull/10634/files#diff-5c2525ef082142486a945b9138611abe282cc048803c89059207112f8db192b8R103-R106
To deal with any type of
ScalableSymbolicPulse
, I need to figure out why I getPulse envelope expression is not assigned
errors after instantiating aScalableSymbolicPulse(pulse_type=some-string,...)
Thank you for the example on
replace()
.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.
Copying a
ScalableSymbolicPulse
is a bit awkward. It should work if you copy all the input parameters:It would be nice to be able to copy the pulse and mutate the
amp
, but mutation is not part of the design. ASymbolicPulse.copy_with_new_parameters()
method might be nice to have.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.
Also, I agree that the current version is fine if you don't want to support
ScalableSymbolicPulse
but I would change theisinstance(<pulse>, Drag)
to<pulse>.pulse_type == "Drag"
to avoid the deprecation ofisinstance
support for the library pulses.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.
Thank you for the discussion 😊 I updated the Drag check according to your suggestion. (In other words, the current version supports only the calibrations with a single Drag pulse)
As a next step, I will support calibrations with a single
ScalableSymbolicPulse
.The final step would be to support calibrations with multiple instructions (nested
ScheduleBlock
, non-Play instructions, etc). However, such calibrations seem unlikely to occur. Moreover, I may need to introduce some arbitrary rules to reject edge cases. Therefore, I would like to require insupported()
that sx calibration should consist of a singleScalableSymbolicPulse
.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 guess current logic causes an edge case. For example,
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.
In that case, more than one value will return
True
fromnp.isclose
. In such a case, I will take only the first value.(Note: the
np.isclose
test for1.235
will return at least oneTrue
, becausertol
is set to a small, nonzero default value. I will retain this setting because settingrtol
to zero can lead to thenp.isclose
returning noTrue
at all, due to uncertainties related to floating-point arithmetic.)