Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix Target.instruction_supported when target.num_qubits == None #13655

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jan 13, 2025

Summary

This PR fixes a small bug found in Target.instruction_supported, where target.num_qubits == None would cause Target.instruction_supported to always evaluate to False. This was handled in the original Python implementation by artificially overwriting qargs, but this would require a mutable variable in Rust, so I just added an additional condition to account for this case.

Details and comments

Bug discovered through #12850.

…um_qubits==None (restore pre-Rust-migration behavior)
@ElePT ElePT added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jan 13, 2025
@ElePT ElePT requested a review from a team as a code owner January 13, 2025 14:20
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@ElePT ElePT added this to the 1.3.2 milestone Jan 13, 2025
@ElePT ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jan 13, 2025
Copy link
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

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

LGTM but before I can approve it, I have one question. Would it be easier to just discard the qargs argument at the beginning the way it was done in Python? You could avoid the mutable reference by just re-defining the variable, for example:

// Handle case where num_qubits is None by always checking globally supported operations
// I don't think it has to be mut, but in case it's needed.
let mut qargs: Option<Qargs> = if self.num_qubits.is_none() {
    None
} else {
    qargs
};

I do see why this would be less clean but it would avoid having to check the same condition multiple times.

@raynelfss raynelfss self-assigned this Jan 13, 2025
@ElePT
Copy link
Contributor Author

ElePT commented Jan 13, 2025

LGTM but before I can approve it, I have one question. Would it be easier to just discard the qargs argument at the beginning the way it was done in Python? You could avoid the mutable reference by just re-defining the variable.

Sure, I was in between the two approaches, and I don't really have a strong preference, so I can totally change it.

@coveralls
Copy link

coveralls commented Jan 13, 2025

Pull Request Test Coverage Report for Build 12750239425

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.5%) to 88.442%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 93.18%
crates/qasm2/src/lex.rs 5 92.73%
crates/qasm2/src/parse.rs 6 97.62%
Totals Coverage Status
Change from base Build 12744574228: -0.5%
Covered Lines: 53802
Relevant Lines: 60833

💛 - Coveralls

Copy link
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

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

LGTM, I wanted to add that this was being done in py_instruction_supported already, but when I split off these functions to have a rust-native parallel, the condition got lost for the rust-native one.

@ElePT ElePT added this pull request to the merge queue Jan 13, 2025
Merged via the queue into Qiskit:main with commit 79f0e72 Jan 13, 2025
17 checks passed
mergify bot pushed a commit that referenced this pull request Jan 13, 2025
…13655)

* Fix Target.instruction_supported to correctly evaluate targets with num_qubits==None (restore pre-Rust-migration behavior)

* Apply Ray's suggestion

* Add reno

(cherry picked from commit 79f0e72)
github-merge-queue bot pushed a commit that referenced this pull request Jan 14, 2025
…13655) (#13657)

* Fix Target.instruction_supported to correctly evaluate targets with num_qubits==None (restore pre-Rust-migration behavior)

* Apply Ray's suggestion

* Add reno

(cherry picked from commit 79f0e72)

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants