Improve performance of RB circuit generation #1317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
itoko
reviewed
Nov 15, 2023
There was a problem hiding this comment.
Thank you @eendebakpt . This is a simple but good improvement I'd like to have. The use of QuantumCircuit._append now sounds good to me since I've heard it's a semi-public API, which should be stable. I just suggest a minor improvement for the correct use of the API.
| @@ -291,7 +291,7 @@ def _sequences_to_circuits( | |||
| circ = QuantumCircuit(self.num_qubits) | |||
| for elem in seq: | |||
| circ.append(self._to_instruction(elem, basis_gates), circ.qubits) | |||
| circ.append(Barrier(self.num_qubits), circ.qubits) | |||
| circ._append(Barrier(self.num_qubits), circ.qubits, circ.cregs) | |||
There was a problem hiding this comment.
Suggested change
| circ._append(Barrier(self.num_qubits), circ.qubits, circ.cregs) | |
| circ._append(CircuitInstruction(Barrier(circ.num_qubits), tuple(circ.qubits), ())) |
with the import of CircuitInstruction
There was a problem hiding this comment.
@itoko No problem to change this, but why is this the correct usage of the API? The type annotation for _append allows for both a CircuitInstruction or Instruction to be passed.
There was a problem hiding this comment.
Excuse me for my short explanation. The old code does not take cargs so it should be empty for drop-in replacement as you've already fixed so. Even if we want to change the spec and add barrier to clbits it should be circ.clbits: Sequence[Clbit] but not circ.cregs: list[ClassicalRegister]. That's why I say the above change is not correct.
As you say the use of Instruction is allowed but the new-style (using CircuitInstruction) would be preferable.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
itoko
pushed a commit
to itoko/qiskit-experiments
that referenced
this pull request
Dec 12, 2023
### Summary In this PR we improve performance of the RB circuit generation. ### Details and comments The main performance bottleneck is the repeat application of `append` of sequence elements to a circuit. We can improve performance by using the `_append` for the `Barrier` gate. For the appending of the Clifford gates we could also use `_append` (in particular if the sequence elements are integral, in which case the internal generation using `_to_instruction` guarantees the elements to not need checking), but since the `StandardRB` is subclassed we have no full information about the elements in the sequence and have chosen not to do this. Profiling on main:  PR:  ### PR checklist (delete when all criteria are met) - [x] I have read the contributing guide `CONTRIBUTING.md`. - [x] I have added the tests to cover my changes. - [x] I have updated the documentation accordingly. - [x] I have added a release note file using `reno` if this change needs to be documented in the release notes. Internal change, no release notes needed
nkanazawa1989
pushed a commit
to nkanazawa1989/qiskit-experiments
that referenced
this pull request
Jan 17, 2024
### Summary In this PR we improve performance of the RB circuit generation. ### Details and comments The main performance bottleneck is the repeat application of `append` of sequence elements to a circuit. We can improve performance by using the `_append` for the `Barrier` gate. For the appending of the Clifford gates we could also use `_append` (in particular if the sequence elements are integral, in which case the internal generation using `_to_instruction` guarantees the elements to not need checking), but since the `StandardRB` is subclassed we have no full information about the elements in the sequence and have chosen not to do this. Profiling on main:  PR:  ### PR checklist (delete when all criteria are met) - [x] I have read the contributing guide `CONTRIBUTING.md`. - [x] I have added the tests to cover my changes. - [x] I have updated the documentation accordingly. - [x] I have added a release note file using `reno` if this change needs to be documented in the release notes. Internal change, no release notes needed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
In this PR we improve performance of the RB circuit generation.
Details and comments
The main performance bottleneck is the repeat application of
appendof sequence elements to a circuit. We can improve performance by using the_appendfor theBarriergate.For the appending of the Clifford gates we could also use
_append(in particular if the sequence elements are integral, in which case the internal generation using_to_instructionguarantees the elements to not need checking), but since theStandardRBis subclassed we have no full information about the elements in the sequence and have chosen not to do this.Profiling on main:
PR:
PR checklist (delete when all criteria are met)
CONTRIBUTING.md.renoif this change needs to be documented in the release notes. Internal change, no release notes needed