Skip to content

Conversation

jakelishman
Copy link
Member

Summary

Commit e2674ce (gh-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.

Details and comments

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 jakelishman added the Changelog: None Do not include in changelog label Jul 13, 2023
@jakelishman jakelishman added this to the 0.25.0 milestone Jul 13, 2023
@jakelishman jakelishman requested a review from a team as a code owner July 13, 2023 14:45
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5544528321

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • 23 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.04%) to 86.018%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/tools/pi_check.py 1 91.23%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 1 90.39%
crates/accelerate/src/vf2_layout.rs 3 94.74%
qiskit/pulse/library/waveform.py 3 93.75%
crates/qasm2/src/lex.rs 4 90.89%
crates/accelerate/src/sabre_swap/layer.rs 5 96.64%
crates/qasm2/src/parse.rs 6 97.58%
Totals Coverage Status
Change from base Build 5543757430: 0.04%
Covered Lines: 71905
Relevant Lines: 83593

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this fixes the oversight in the previous PR, one inline question but not a blocker since it existed before.

return False
qubits = tuple(self.qubits.index(qubit) for qubit in instruction.qubits)
qubits = tuple(self.qubits.index(qubit) for qubit in qubits)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mtreinish mtreinish added this pull request to the merge queue Jul 13, 2023
Merged via the queue into Qiskit:main with commit 7e24c16 Jul 13, 2023
@jakelishman jakelishman deleted the fix-has-calibration-for-tuple branch July 13, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants