-
Notifications
You must be signed in to change notification settings - Fork 126
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
Delay support in Interleaved Randomized Benchmarking #776
Conversation
if hasattr(group_elt_gate, "to_gate"): | ||
group_elt_gate = group_elt_gate.to_gate() | ||
if not isinstance(group_elt_gate, Instruction): | ||
group_elt_gate = group_elt_gate.to_instruction() |
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 this has been already resolved with #762. Currently test is missing because support of Delay requires the latest terra which was unstable at the time I merged the PR. What is the point using to_instruction
?
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.
The fix in #762 does not cover the case of interleaving a circuit with delays, which is tested in the test case below. If we interleave a QuantumCircuit object with delay, it fails in QuantumCircuit.to_gate
method (as Delay is not a gate).
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.
Make sense. I've forgotten the interleaved element typehint says QuantumCircuit
is acceptable.
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.
LGTM
releasenotes/notes/add-delay-support-in-irb-ae090968aadd7a54.yaml
Outdated
Show resolved
Hide resolved
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.
Looks straightforward, but I think this should count as a bugfix.
I think the tests require fixes from terra 0.20.1, so could you confirm that, and if so update the minimum terra req?
releasenotes/notes/add-delay-support-in-irb-ae090968aadd7a54.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/add-delay-support-in-irb-ae090968aadd7a54.yaml
Outdated
Show resolved
Hide resolved
@@ -345,6 +346,39 @@ def test_non_clifford_interleaved_element(self): | |||
lengths=lengths, | |||
) | |||
|
|||
def test_interleaving_delay(self): |
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 both these test requires qiskit-terra 0.20.1 to not error
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.
Good point. All test passed in CI because it simply builds env with the latest version?
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.
Yeah it will pull the latest stable in CI, but if someone updates an existing qiskit-experiment install from pip it wont automatically update terra unless we change the requirements.
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.
Updated the minimum requirements in c5e66cd
Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>
Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>
…y#776) Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com> Co-authored-by: Toshinari Itoko <itoko@jp.ibm.com> Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>
Summary
Make Interleaved randomized benchmarking interleave the 'delay' instruction
Details and comments
Fixes #727 . Requires a fix in qiskit-terra : Qiskit/qiskit-terra#7850