Skip to content
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

Solve strange behavior with qubit_properties in Target. #13400

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

raynelfss
Copy link
Contributor

Summary

Fixes #13393.

The following commits address some strange behavior when deciding the num_qubits attribute of a Target when only qubit_properties is provided.

Details and comments

The previous implementation of the Target would always make sure to receive a real value for num_qubits or it would default to zero. Mistakenly because of the behavior of Target::from_configuration it was assumed that num_qubits could be None unless otherwise specified. However, the default behavior should be to make num_qubits zero unless specified otherwise.

The following commits fix this behavior by fixing the signature of the rust Target to have default values and adding an extra check when comparing against qubit_properties.

A test case based on @wshanks findings in #13393 was also included in these additions.

The previous implementation of the Target would always make sure to receive a real value for `num_qubits` or it would default to zero. Mistakenly because of the behavior of `Target::from_configuration` it was assumed that `num_qubits` could be `None` unless otherwise specified. However, the default behavior should be to make `num_qubits` zero unless specified otherwise.

The following commits fix this behavior by fixing the signature of the rust `Target` to have default values and adding an extra check when comparing against `qubit_properties`.

A test casde based on @wshanks findings in Qiskit#13393 was also included in these additions.
@raynelfss raynelfss added bug Something isn't working Changelog: None Do not include in changelog labels Nov 5, 2024
@raynelfss raynelfss added this to the 1.3.0 milestone Nov 5, 2024
@raynelfss raynelfss requested a review from a team as a code owner November 5, 2024 21:22
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11692746207

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 7 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.01%) to 88.808%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
crates/accelerate/src/two_qubit_decompose.rs 1 92.09%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 1 95.38%
crates/qasm2/src/lex.rs 4 92.48%
Totals Coverage Status
Change from base Build 11690680453: 0.01%
Covered Lines: 77066
Relevant Lines: 86778

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Hmmm I am a bit weary of the change in the default value of num_qubits because I remember playing around with it with the refactoring of BasicSimulator and concluding that the default should stay None. However, it's true that the BasicSimulator target is instantiated explicitly with num_qubits=None, so maybe I remember a different issue. In any case, wouldn't this have to be documented as an interface change? I don't think it should be something users heavily rely on but I think this is part of the user-facing API, right?

@mtreinish
Copy link
Member

I think the switch to None was an accidentally breaking change that I missed in the review of #12292 looking at an old version of the target code it defaulted to 0: https://github.com/Qiskit/qiskit/blob/stable/1.1/qiskit/transpiler/target.py#L247 although reviewing #12292 now it looks like the python side constructor still used 0, but the rust used None

@ElePT
Copy link
Contributor

ElePT commented Nov 6, 2024

Aha, then maybe I remembered the issue the other way round, the default was 0 and I thought of changing it to None, and that broke some code down the line, so I just did it explicitly in the BasicSimulator construction. Then @raynelfss disregard my previous comment.

@raynelfss raynelfss assigned ElePT and unassigned jakelishman Nov 6, 2024
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Ok, double checking the historical target version @mtreinish linked, I think it makes sense to align the default values of the constructor with the python ones, both for num_qubits and the pulse-related args. LGTM!

@ElePT ElePT added this pull request to the merge queue Nov 6, 2024
Merged via the queue into Qiskit:main with commit 6ed7743 Nov 6, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Changelog: None Do not include in changelog priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Target raises exception when qubit_properties is used without passing num_qubits
6 participants