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

Followup of #11095 #11404

Merged
merged 5 commits into from
Jan 18, 2024
Merged

Conversation

nkanazawa1989
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 commented Dec 12, 2023

Summary

This PR is followup of #11095 to address comments from @mtreinish along with a bugfix. The bug is reported in Qiskit Experiments, in which backend version up-conversion fails when the value of gate config is invalid. This is an edge case of the FakeOpenPulse2Q backend, because its gate config doesn't match with the basis gates that it reports. However this must be resolved.

Details and comments

Code cleanup + Bugfix

Several cleanup is done in this PR.

  • Typehint fix
  • Handling of faulty qubits
  • Direct instantiation of instruction properties object
  • More inline comments for future developer

- Add exception handling for the edge case in which a basis gate property is not reported
- Cleanup docs
- Replace logging with RuntimeWarning
- Add more inline comments
- Fix wrong typehints
- Update handling of faulty qubits with set operation
@nkanazawa1989 nkanazawa1989 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Dec 12, 2023
@nkanazawa1989 nkanazawa1989 requested review from jyu00 and a team as code owners December 12, 2023 19:44
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

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.

Overall this LGTM it's a nice cleanup, just a couple small questions inline

else:
# Create inst properties placeholder
for name in all_instructions:
if name not in prop_name_map:
Copy link
Member

Choose a reason for hiding this comment

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

Can this ever be True? all_instructions is a set so there aren't any duplicates and prop_name_map on the first iteration is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Updated initialization of prop_name_map

qiskit/providers/backend_compat.py Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 13, 2023

Pull Request Test Coverage Report for Build 7531813412

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.8%) to 89.376%

Totals Coverage Status
Change from base Build 7198837138: 1.8%
Covered Lines: 59510
Relevant Lines: 66584

💛 - 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.

This LGTM, my only question before approving is around the release notes from #11095. Did we add the upgrade notes about the api changes, see:

#11095 (comment) and #11095 (comment) (they might have been done in another PR and I just forgot during my winter vacation)

@nkanazawa1989
Copy link
Contributor Author

Good catch! Updated the reno in 5f7bc6e (also rephrased a bit).

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.

Overall this LGTM, just one tiny issue inline in the logic around adding required instructions to the target.

# Map required ops to each operational qubit
if prop_name_map[op] is None:
prop_name_map[op] = {
(q,): None for q in range(configuration.num_qubits) if q not in faulty_qubits
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we only exclude the faulty qubits here if filter_faulty is True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in bcb4144 (also added a test).

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 now, thanks for the quick update

@mtreinish mtreinish added this pull request to the merge queue Jan 18, 2024
Merged via the queue into Qiskit:main with commit 828ad4d Jan 18, 2024
13 checks passed
ihincks pushed a commit to ihincks/qiskit-terra that referenced this pull request Jan 19, 2024
* Followup
- Add exception handling for the edge case in which a basis gate property is not reported
- Cleanup docs
- Replace logging with RuntimeWarning
- Add more inline comments
- Fix wrong typehints
- Update handling of faulty qubits with set operation

* bugfix + more warning message

* Update reno

* Add more check for filter option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants