Allow dynamical decoupling pass to work on circuits with pulse gates #10834

wshanks · 2023-09-13T15:36:43Z

Summary

Allow dynamical decoupling pass to work on circuits with pulse gates

Details and comments

The pass was accessing the circuit calibrations incorrectly, resulting in
an error when one of the gates in the dynamical decoupling sequence had
a pulse gate calibration.

Closes #10833

qiskit-bot · 2023-09-13T15:36:47Z

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

@Qiskit/terra-core

wshanks · 2023-09-13T15:36:57Z

Closes #10833

jakelishman

In principle this looks fine to me, though I'd wait for one of the pulse maintainers to review properly.

jakelishman · 2023-09-13T15:42:38Z

qiskit/transpiler/passes/scheduling/padding/dynamical_decoupling.py

-                    gate_length = dag.calibrations[gate.name][(physical_index, gate.params)]
+                    gate_length = dag.calibrations[gate.name][
+                        ((physical_index,), tuple(gate.params))
+                    ].duration


In QuantumCircuit.has_calibration_for, the params tuple is built by coercing bound ParameterExpression objects to float as well:

qiskit/qiskit/circuit/quantumcircuit.py

Lines 451 to 456 in e599713

params = []

for p in operation.params:

if isinstance(p, ParameterExpression) and not p.parameters:

params.append(float(p))

else:

params.append(p)

Does that need to be done here too?

I think you are right. I think it would probably be best to have a QuantumCircuit.get_calibration(gate, qubits, params) method to abstract this operation, but I won't add that here.

Yeah, agreed on both parts.

jakelishman · 2023-09-13T15:44:32Z

You need to put the "closes #10833" in the top PR comment for it to have an effect, but you can edit it in and it'll work.

wshanks · 2023-09-13T15:47:15Z

Ah, okay. I put it separately because I don't like it ending up in the commit message, but I guess it's fine.

nkanazawa1989

Thanks Will. This looks good to me. I'll approve and add this to queue once linter error is fixed.

nkanazawa1989 · 2023-09-13T15:53:58Z

test/python/transpiler/test_dynamical_decoupling.py

@@ -17,6 +17,7 @@
 from numpy import pi
 from ddt import ddt, data

+from qiskit import pulse


Imported twice

nkanazawa1989 · 2023-09-13T15:55:45Z

test/python/transpiler/test_dynamical_decoupling.py

@@ -339,6 +340,69 @@ def test_insert_dd_ghz_everywhere(self):

        self.assertEqual(ghz4_dd, expected)

+    def test_insert_dd_with_pulse_gate_calibrations(self):


Do you also want a test for parametric echo gate? I don't think this is realistic and it's up to you.

Since the code handles parameters, we might as well test them. I added an extra test.

jakelishman · 2023-09-13T16:07:35Z

I put it separately because I don't like it ending up in the commit message, but I guess it's fine.

The PR body isn't used for the commit message in a squash-merge - GitHub just catenates all the commit messages together, so if you've got stuff that's added to the PR comment but not the head commit message, it doesn't get included. (I think the rules are different if the PR is merged by merge, though.)

wshanks · 2023-09-14T03:38:45Z

The PR body isn't used for the commit message in a squash-merge - GitHub just catenates all the commit messages together, so if you've got stuff that's added to the PR comment but not the head commit message, it doesn't get included. (I think the rules are different if the PR is merged by merge, though.)

Ah, okay, I have not looked closely at the commit messages since this repo switched to using the merge queue (EDIT: actually, I see now that mergify was also configured for this commit message format, but back then one could also do a direct "Squash and merge" though one was not supposed to because it require extra CI churn). In qiskit-experiments, when we started using the merge queue, we changed the default commit message to "Pull request title and body" so that we could have more control over the commit message. I forgot that wasn't the default behavior. I hope some day the "Merge when ready" button will open a text box for editing the commit message like "Squash and merge" does.

Now I want to squash and force push before merging 🤔

coveralls · 2023-09-14T03:58:09Z

Pull Request Test Coverage Report for Build 6180555583

10 of 11 (90.91%) changed or added relevant lines in 1 file are covered.
23 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.02%) to 87.262%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/scheduling/padding/dynamical_decoupling.py	10	11	90.91%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	5	92.17%
crates/qasm2/src/parse.rs	18	96.67%

Totals
Change from base Build 6174101621:	-0.02%
Covered Lines:	74199
Relevant Lines:	85030

💛 - Coveralls

The pass was accessing the circuit calibrations incorrectly, resulting in an error when one of the gates in the dynamical decoupling sequence had a pulse gate calibration.

nkanazawa1989

Thanks. This looks good to me.

…10834) The pass was accessing the circuit calibrations incorrectly, resulting in an error when one of the gates in the dynamical decoupling sequence had a pulse gate calibration. (cherry picked from commit 4e49a56)

…10834) (#10848) The pass was accessing the circuit calibrations incorrectly, resulting in an error when one of the gates in the dynamical decoupling sequence had a pulse gate calibration. (cherry picked from commit 4e49a56) Co-authored-by: Will Shanks <willshanks@us.ibm.com>

wshanks requested a review from nkanazawa1989 September 13, 2023 15:36

wshanks requested a review from a team as a code owner September 13, 2023 15:36

jakelishman reviewed Sep 13, 2023

View reviewed changes

nkanazawa1989 reviewed Sep 13, 2023

View reviewed changes

wshanks force-pushed the dd-pg branch 2 times, most recently from 41d98fe to da6b511 Compare September 14, 2023 20:06

wshanks added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: pulse Related to the Pulse module mod: transpiler Issues and PRs related to Transpiler labels Sep 14, 2023

Allow dynamical decoupling pass to work on circuits with pulse gates

9f45680

The pass was accessing the circuit calibrations incorrectly, resulting in an error when one of the gates in the dynamical decoupling sequence had a pulse gate calibration.

wshanks force-pushed the dd-pg branch from da6b511 to 9f45680 Compare September 14, 2023 20:08

wshanks requested a review from nkanazawa1989 September 14, 2023 22:09

nkanazawa1989 approved these changes Sep 15, 2023

View reviewed changes

nkanazawa1989 added this pull request to the merge queue Sep 15, 2023

Merged via the queue into Qiskit:main with commit 4e49a56 Sep 16, 2023

mergify bot mentioned this pull request Sep 16, 2023

Allow dynamical decoupling pass to work on circuits with pulse gates (backport #10834) #10848

Merged

wshanks deleted the dd-pg branch September 18, 2023 15:32

	params = []
	for p in operation.params:
	if isinstance(p, ParameterExpression) and not p.parameters:
	params.append(float(p))
	else:
	params.append(p)

		@@ -339,6 +340,69 @@ def test_insert_dd_ghz_everywhere(self):

		self.assertEqual(ghz4_dd, expected)

		def test_insert_dd_with_pulse_gate_calibrations(self):

Allow dynamical decoupling pass to work on circuits with pulse gates #10834

Allow dynamical decoupling pass to work on circuits with pulse gates #10834

Uh oh!

Conversation

wshanks commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Sep 13, 2023

Uh oh!

wshanks commented Sep 13, 2023

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

jakelishman Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

wshanks Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

jakelishman Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

jakelishman commented Sep 13, 2023

Uh oh!

wshanks commented Sep 13, 2023

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

wshanks Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

wshanks Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

jakelishman commented Sep 13, 2023

Uh oh!

wshanks commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 14, 2023

Pull Request Test Coverage Report for Build 6180555583

💛 - Coveralls

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wshanks commented Sep 13, 2023 •

edited

Loading

wshanks commented Sep 14, 2023 •

edited

Loading