Fix API break in QuantumCircuit.has_calibration_for#10427
Conversation
Commit e2674ce (Qiskitgh-10416) reduced usage of the legacy format for `CircuitInstruction` within Terra, but one of the changes meant that a previously valid input to a public API function became invalid. This caused Qiskit Experiments' CI to fail, and wasn't a valid change to Terra.
jakelishman
added
the
Changelog: None
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5544528321
💛 - Coveralls |
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
LGTM, this fixes the oversight in the previous PR, one inline question but not a blocker since it existed before.
| if not self.calibrations or operation.name not in self.calibrations: | ||
| return False | ||
| qubits = tuple(self.qubits.index(qubit) for qubit in instruction.qubits) | ||
| qubits = tuple(self.qubits.index(qubit) for qubit in qubits) |
There was a problem hiding this comment.
Should this use find bits? It's the same as before so I'm fine merging it as is, but I just worry the performance of this is pretty poor. Although I guess the number of qubits for a gate won't be that wide so it's probably not a big deal.
There was a problem hiding this comment.
Yeah it probably should - this is doing unnecessary linear searches now. I think there's a few places throughout Qiskit where we're doing list.index, and they're pretty much all potential performance problems.
There was a problem hiding this comment.
Well lets do all of them in a single pass, the performance here isn't critical for this PR, it's more to fix the api breakage.
4 participants
Summary
Commit e2674ce (gh-10416) reduced usage of the legacy format for
CircuitInstructionwithin Terra, but one of the changes meant that a previously valid input to a public API function became invalid. This caused Qiskit Experiments' CI to fail, and wasn't a valid change to Terra.Details and comments