include ecr native gate for pulse scaling #9397

nbronn · 2023-01-19T22:33:30Z

Summary

The addition of IBM Quantum backends without native cx gates (i.e., ibm_sherbrooke where the native entangling gate is ecr, echoed cross resonance) is not compatible with the pulse scaling done by RZXCalibrationBuilder and RZXCalibrationBuilderNoEcho. This PR addresses this problem by treating the cx and ecr gate on equal footing in those classes.

Details and comments

The logic that handles extracting information from the entangling gates has been moved to _check_calibration_type and the corresponding gate is checked for in backend.defaults().instruction_schedule_map before assigning the correct schedule and corresponding CRCalType (either by definition as it is only assigned in one direction for ecr or an inspection of which qubit the on-resonance compensation tones are applied to).

The unit test TestCalibrationBuilder is also updated for compatibility with the same logic, however the FakeSherbrooke backend from qiskit.providers.fake_provider currently does not have a backend.defaults() method to obtain the instruction schedule map from.

…e-ecr-gate-for-pulse-scaling

…se provided by ibm_sherbrooke

qiskit-bot · 2023-01-19T22:33:38Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core

coveralls · 2023-01-19T23:02:07Z

Pull Request Test Coverage Report for Build 4055132298

30 of 39 (76.92%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.04%) to 85.223%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/calibration/rzx_builder.py	27	36	75.0%

Totals
Change from base Build 4055127961:	-0.04%
Covered Lines:	66835
Relevant Lines:	78424

💛 - Coveralls

nkanazawa1989

Thanks @nbronn I like the new design of CRCalType enum. I think you also need to write a release note (as new feature).

qiskit/transpiler/passes/calibration/rzx_builder.py

nkanazawa1989 · 2023-01-20T12:28:21Z

test/python/transpiler/test_calibrationbuilder.py

@@ -57,15 +57,21 @@ def setUp(self):
        def _get_cr(time_inst, name):
            return isinstance(time_inst[1], Play) and time_inst[1].pulse.name.startswith(name)

-        cx_sched = self.inst_map.get("cx", (0, 1))
+        qubits = (0, 1)


Why this must be changed? This instmap is the past snapshot of Hanoi which must be the forward CX. You need to upgrade this test class with ddt to run both on (fake) Hanoi and (fake) Sherbrooke to test CX and ECR.

I originally changed cx_sched to cr_sched to treat the native entangling gates on equal footing. But I think you are correct in that I would need to make a separate test class with ddt to test the ecr case with (fake) Sherbrooke. Might it make more sense to run the same code for the two cases of backends (also this assumes Hanoi will always default to cx and Sherbrooke to ecr)?

ddt with two backends sounds better, but you need overhaul of the test class because it is currently tied to Hanoi. You can remove setup and add a method to get individual play instruction, e.g. self.u0p_play -> def get_u0p_play(self, backend): and then pass the backend with ddt at each test function.

Hanoi and Sherbrooke might change in future, but fake backend data is locally stored as a part of terra and the object is built from there. So the gate will be always cx for Hanoi (and I guess ecr for Sherbrooke) unless someone manually update the backend data (there is a command to update the data).

…e-ecr-gate-for-pulse-scaling

nkanazawa1989 · 2023-01-23T09:42:29Z

fix #9308

…e-ecr-gate-for-pulse-scaling

…kend/versions

mtreinish · 2023-01-25T23:27:02Z

releasenotes/notes/include-ecr-gates-for-pulse-scaling-8369eb584c6d8fe1.yaml

+issues:
+  - |
+    Currently the FakeSherbrooke backend does not contain `.defaults()` from which
+    the instruction schedule map is obtained. The unit test `TestCalibrationBuilder` 
+    requires this to check `ecr` operations work similarly to the `cx` gate case. 
+    As of this PR, `ibm_sherbrooke` is the only backend with native `ecr` gates, so 
+    this requires updating. 


I think this should be resolved. The FakeSherbrooke backend is based on BackendV2 which doesn't have configuration(), properties(), or defaults() methods. The calibrations for gates are part of the Target object (backend.target) and you can get an InstructionScheduleMap from a target (backend.target.instruction_schedule_map()). This pass was updated in #9343 to take in a target directly so when you run with FakeSherbrooke you should just be able to do:

from qiskit.transpiler.passes import RZXCalibrationBuilder from qiskit.providers.fake_provider import FakeSherbrooke cal_pass = RZXCalibrationBuilder(target=FakeSherbrooke().target)

This is not that easy. We need to build reference schedule to test, but currently the builder is tied to the V1 accessors and we cannot generate the schedule with FakeV2. I wrote this issue and assigned a team member to it. We plan to update the builder in 0.24, and then we can write a test with FakeSherbrooke. The plan is (since @nbronn needs this code in a few weeks) to wait for 0.23 release and merge this PR without test. Then add the test with followup PR after above issue is resolved.

@nbronn You can remove this issue note. I don't think end-users have concern about unit-test. You can leave this as a comment in the unit-test file so that developer can be aware of this.

nkanazawa1989 · 2023-01-26T17:06:05Z

test/python/transpiler/test_calibrationbuilder.py

@@ -88,6 +230,93 @@ def compute_stretch_width(self, play_gaussian_square_pulse, theta):
        target_area = abs(theta) / self.__angle * full_area
        return max(0, target_area - risefall_area)

+    def test_native_cr(self):


This must be a part of TestRZXCalibrationBuilder because this class is a superclass of two Rzx passes of echo and non-echo. You need to write tests for

native cr with echo

backward cr with echo

native cr with non-echo (backward for non echo is not supported)

with both FakeHanoi, FakeHanoiV2 and Sherbrooke.

eggerdj

Please also update EchoRZXWeylDecomposition to include the ECR.

eggerdj · 2023-01-26T18:39:39Z

While we are at it, should we add GaussianSquareDrag to this list: https://github.com/Qiskit/qiskit-terra/blob/925f9f3abfdee4faeae67503082d7877f080c2ed/qiskit/transpiler/passes/calibration/rzx_builder.py#L330 ?

…c in echo_rzx_weyl_decomposition.py

…ion logic in echo_rzx_weyl_decomposition.py" This reverts commit baf069f.

…st of the RZXCalibrationBuilder (because they will need to be copied for different backends)

nbronn · 2023-01-26T23:46:51Z

Please also update EchoRZXWeylDecomposition to include the ECR.

Modified EchoRZXWeylDecomposition to use the same logic as rzx_builder.py.

nbronn · 2023-01-26T23:49:43Z

While we are at it, should we add GaussianSquareDrag to this list:

https://github.com/Qiskit/qiskit-terra/blob/925f9f3abfdee4faeae67503082d7877f080c2ed/qiskit/transpiler/passes/calibration/rzx_builder.py#L330

?

It is my understanding that the GaussianSquareDrag were only used as compensation tones for DirectCX. Additionally, the DirectCX includes extra pulses that should not scale with the CR angle, so including them will take extra consideration that I believe is outside the scope of this PR.

nkanazawa1989 · 2023-01-27T02:06:13Z

If I understand correctly GaussianSquareDrag just adds DRAG component to the risefall and can be applied to ECR to deal with noisy spectrum. So I think adding drag version to validation makes sense, though I'm not sure how we can scale this pulse in small angle regime where we need to renormalize amplitude.

It is my understanding that the GaussianSquareDrag were only used as compensation tones for DirectCX.

I think DirectCX uses GaussianSquareEcho in this PR.

eggerdj · 2023-01-27T09:35:46Z

Replying to #9397 (comment) if you remove the flat-top part you have a normal Gaussian, you can still scale the amplitude but you might have some miscalibrations you need to compensate for. This is anyhow the case for Gaussian square without the drag.

… native gates

nkanazawa1989 · 2023-01-30T01:12:15Z

Yes. The question is how we scale the drag component in general. Since this is proportional to the derivative of amplitude, the drag coefficient must be changed with the amplitude. AFAIK there is no established method for this.

nkanazawa1989

Thanks @nbronn . This almost looks good to me. This is one last feedback for unittest cleanup.

nkanazawa1989 · 2023-01-30T01:24:43Z

releasenotes/notes/include-ecr-gates-for-pulse-scaling-8369eb584c6d8fe1.yaml

+issues:
+  - |
+    Currently the FakeSherbrooke backend does not contain `.defaults()` from which
+    the instruction schedule map is obtained. The unit test `TestCalibrationBuilder` 
+    requires this to check `ecr` operations work similarly to the `cx` gate case. 
+    As of this PR, `ibm_sherbrooke` is the only backend with native `ecr` gates, so 
+    this requires updating. 


@nbronn You can remove this issue note. I don't think end-users have concern about unit-test. You can leave this as a comment in the unit-test file so that developer can be aware of this.

test/python/transpiler/test_calibrationbuilder.py

nkanazawa1989

Thanks @nbronn this looks good to me.

eggerdj

This now has the ECR and Weyl decomposition update. LGTM. Thanks Nick.

nbronn added 3 commits January 17, 2023 17:05

added fake non-working example of dynamic replacment pass

a9df820

Merge branch 'main' of github.com:qiskit/qiskit-terra into bug/includ…

bbc7ffa

…e-ecr-gate-for-pulse-scaling

modified RZXCalibrationBuilder to accept ecr native gates such as tho…

8f31790

…se provided by ibm_sherbrooke

nbronn requested a review from a team as a code owner January 19, 2023 22:33

nkanazawa1989 added the mod: pulse Related to the Pulse module label Jan 20, 2023

nkanazawa1989 self-assigned this Jan 20, 2023

nkanazawa1989 requested changes Jan 20, 2023

View reviewed changes

nbronn added 2 commits January 20, 2023 12:12

Merge branch 'main' of github.com:qiskit/qiskit-terra into bug/includ…

0c22129

…e-ecr-gate-for-pulse-scaling

made fixes asked for in the PR review

c303979

nbronn added 2 commits January 23, 2023 07:37

Merge branch 'main' of github.com:qiskit/qiskit-terra into bug/includ…

6522d5f

…e-ecr-gate-for-pulse-scaling

modifying and copying tests of calibration builder with different bac…

56872e7

…kend/versions

mtreinish reviewed Jan 25, 2023

View reviewed changes

nkanazawa1989 added the on hold Can not fix yet label Jan 26, 2023

nkanazawa1989 mentioned this pull request Jan 26, 2023

Add ECR to calibration builders #9467

Closed

nkanazawa1989 reviewed Jan 26, 2023

View reviewed changes

eggerdj requested changes Jan 26, 2023

View reviewed changes

nbronn added 5 commits January 26, 2023 16:45

extended DCX to forward and reverse types and included direction logi…

baf069f

…c in echo_rzx_weyl_decomposition.py

fixed merge conflicts between this PR and main

c7f6ad6

Revert "extended DCX to forward and reverse types and included direct…

572c347

…ion logic in echo_rzx_weyl_decomposition.py" This reverts commit baf069f.

included new logic for finding CR direction

9fb8c78

fixed tox / lint issues - incl removing references to self in many te…

072da9d

…st of the RZXCalibrationBuilder (because they will need to be copied for different backends)

whoops... minor linting error

82a215b

nbronn added 4 commits January 27, 2023 09:26

added class methods for returning pulse schedules needed for testing

f93b565

put additional test in the right place (TODOs) for BackendV2s and ecr…

b413cf3

… native gates

fixed minor linting error

8442da3

added missing docstrings

680d6bf

nkanazawa1989 requested changes Jan 30, 2023

View reviewed changes

nbronn added 2 commits January 30, 2023 15:26

removed issue about backendv2s from release notes

c7fb5bf

made changes to unit testing asked for in PR

3ba85cc

nkanazawa1989 approved these changes Jan 31, 2023

View reviewed changes

eggerdj approved these changes Jan 31, 2023

View reviewed changes

nkanazawa1989 added automerge and removed on hold Can not fix yet labels Jan 31, 2023

Merge branch 'main' into bug/include-ecr-gate-for-pulse-scaling

1b58d6a

mergify bot merged commit 5108380 into Qiskit:main Jan 31, 2023

nkanazawa1989 mentioned this pull request Mar 30, 2023

Support ECR gate in calibration builders #9308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include ecr native gate for pulse scaling #9397

include ecr native gate for pulse scaling #9397

nbronn commented Jan 19, 2023

qiskit-bot commented Jan 19, 2023

coveralls commented Jan 19, 2023 •

edited

Loading

nkanazawa1989 left a comment

nkanazawa1989 Jan 20, 2023 •

edited

Loading

nbronn Jan 20, 2023

nkanazawa1989 Jan 21, 2023 •

edited

Loading

nkanazawa1989 commented Jan 23, 2023

mtreinish Jan 25, 2023

nkanazawa1989 Jan 26, 2023

nkanazawa1989 Jan 30, 2023

nkanazawa1989 Jan 26, 2023

eggerdj left a comment

eggerdj commented Jan 26, 2023

nbronn commented Jan 26, 2023

nbronn commented Jan 26, 2023

nkanazawa1989 commented Jan 27, 2023 •

edited

Loading

eggerdj commented Jan 27, 2023

nkanazawa1989 commented Jan 30, 2023

nkanazawa1989 left a comment

nkanazawa1989 Jan 30, 2023

nkanazawa1989 left a comment

eggerdj left a comment

include ecr native gate for pulse scaling #9397

include ecr native gate for pulse scaling #9397

Conversation

nbronn commented Jan 19, 2023

Summary

Details and comments

qiskit-bot commented Jan 19, 2023

coveralls commented Jan 19, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4055132298

💛 - Coveralls

nkanazawa1989 left a comment

Choose a reason for hiding this comment

nkanazawa1989 Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

nbronn Jan 20, 2023

Choose a reason for hiding this comment

nkanazawa1989 Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

nkanazawa1989 commented Jan 23, 2023

mtreinish Jan 25, 2023

Choose a reason for hiding this comment

nkanazawa1989 Jan 26, 2023

Choose a reason for hiding this comment

nkanazawa1989 Jan 30, 2023

Choose a reason for hiding this comment

nkanazawa1989 Jan 26, 2023

Choose a reason for hiding this comment

eggerdj left a comment

Choose a reason for hiding this comment

eggerdj commented Jan 26, 2023

nbronn commented Jan 26, 2023

nbronn commented Jan 26, 2023

nkanazawa1989 commented Jan 27, 2023 • edited Loading

eggerdj commented Jan 27, 2023

nkanazawa1989 commented Jan 30, 2023

nkanazawa1989 left a comment

Choose a reason for hiding this comment

nkanazawa1989 Jan 30, 2023

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

eggerdj left a comment

Choose a reason for hiding this comment

coveralls commented Jan 19, 2023 •

edited

Loading

nkanazawa1989 Jan 20, 2023 •

edited

Loading

nkanazawa1989 Jan 21, 2023 •

edited

Loading

nkanazawa1989 commented Jan 27, 2023 •

edited

Loading