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

Perform fractional gate filtering on raw API dicts #2044

Merged
merged 17 commits into from
Nov 24, 2024

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Nov 15, 2024

This change moves the filtering of fractional gates and dynamic circuit instructions up into the processing of the raw configuration and properties dicts received through the API. Previously, the filtering was performed only during the creation of the Target for the backend. So previously an IBMBackend with fractional gates would have both fractional gates and dynamic circuit instructions in its configuration and properties data regardless of what value was used for use_fractional_gates. This situation led to inconsistent behavior when for example users tried to use backend.basis_gates as the reference for which gates were available instead of
backend.target.operation_names.

The previous changes to filter instructions during target creation are removed here, but the use_fractional_gates backend option is retained because IBMBackend loads the properties data lazily and can reload the configuration data after creation with the refresh() method.

Closes #2031

This change moves the filtering of fractional gates and dynamic circuit
instructions up into the processing of the raw configuration and
properties dicts received through the API. Previously, the filtering was
performed only during the creation of the `Target` for the backend. So
previously an `IBMBackend` with fractional gates would have both
fractional gates and dynamic circuit instructions in its configuration
and properties data regardless of what value was used for
`use_fractional_gates`. This situation led to inconsistent behavior when
for example users tried to use `backend.basis_gates` as the reference
for which gates were available instead of
`backend.target.operation_names`.

The previous changes to filter instructions during target creation are
removed here, but the `use_fractional_gates` backend option is retained
because `IBMBackend` loads the properties data lazily and can reload the
configuration data after creation with the `refresh()` method.

Closes Qiskit#2031
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Overall the change looks good. I have a couple of concerns as written in inline comments.

qiskit_ibm_runtime/utils/backend_decoder.py Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_decoder.py Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_converter.py Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_decoder.py Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_converter.py Outdated Show resolved Hide resolved
@wshanks
Copy link
Collaborator Author

wshanks commented Nov 18, 2024

I think this is ready for another look.

Copy link
Collaborator

@yaelbh yaelbh left a comment

Choose a reason for hiding this comment

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

Edit the comment in

# Set fractional gate feature before Target object is created.

Another comment to be edited, although it's not from this PR, but related:

"""Create an IBMBackend instance from raw server data.

And, of course, thanks a lot for doing this work 😄

qiskit_ibm_runtime/qiskit_runtime_service.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_decoder.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_decoder.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_decoder.py Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_decoder.py Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_decoder.py Show resolved Hide resolved
qiskit_ibm_runtime/ibm_backend.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@yaelbh yaelbh left a comment

Choose a reason for hiding this comment

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

Added a few small comments, although you didn't say that it's ready for another review

qiskit_ibm_runtime/qiskit_runtime_service.py Outdated Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_converter.py Show resolved Hide resolved
qiskit_ibm_runtime/utils/backend_converter.py Show resolved Hide resolved
Copy link
Collaborator Author

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

Thanks, @yaelbh. I did mean to ask for another review after my last changes. I think it is ready again.

I think I have addressed all the comments, but I didn't mark anything as resolved. Feel free to mark any conversations as resolved if you want to clear up what is still outstanding.

@wshanks wshanks dismissed nkanazawa1989’s stale review November 22, 2024 21:25

Naoki is on leave and we don't want to make him come back early to re-review or wait for him to come back.

@yaelbh yaelbh merged commit af083a4 into Qiskit:main Nov 24, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update basis gates according to the fractional gates flag
3 participants