-
Notifications
You must be signed in to change notification settings - Fork 196
Correct fractional gates configuration check for cached usage of backend object #2547
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
base: main
Are you sure you want to change the base?
Conversation
…l gates are called again
yaelbh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jan-an, it's very good and my comments are small.
| @@ -0,0 +1,3 @@ | |||
| `.QiskitRuntimeService` will now check for both ``rzz`` and ``rx`` gates in the | |||
| backend's basis gates to decide whether to refresh the configuration or fetch | |||
| from cache | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a sentence that explains what's the change here, i.e., describe how it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added sentence (commit) 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yaelbh, thank you for your comments and suggestions! I've made the changes, can you kindly take a look?
| real_device_name = "ibm_miami" | ||
| backend_fg = self.service.backend(real_device_name, use_fractional_gates=True) | ||
| backend_fg2 = self.service.backend(real_device_name, use_fractional_gates=True) | ||
| backend_no_fg = self.service.backend(real_device_name, use_fractional_gates=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding:
backend_no_fg2 = self.service.backend(real_device_name, use_fractional_gates=False)
backend_fg_3 = self.service.backend(real_device_name, use_fractional_gates=True)and the corresponding assertions below. This way all four cases of (previous fg, current fg) will be covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added additional backend objects and assert statements as suggested (commit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages were changed in this last commit, and I think the previous version was the correct one, please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I was writing some comments to follow what each assert was doing but forgot to negate them when I copied them for raising the error message, that's why this happened. Sorry about that, I've corrected the tests and the error messages again, they should be okay now.
Added fullstop Co-authored-by: Yael Ben-Haim <[email protected]>
Included the issue pertaining to the bug in addition to information about the fix in release notes
Added a few more assertions to round off issues we saw in the bug report
| self.assertIsNot( | ||
| backend_no_fg2, backend_fg3, "Configuration refreshes when use_fractional_gates changes" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion can be deleted, it's a direct consequence of the next assertion.
| real_device_name = "ibm_miami" | ||
| backend_fg = self.service.backend(real_device_name, use_fractional_gates=True) | ||
| backend_fg2 = self.service.backend(real_device_name, use_fractional_gates=True) | ||
| backend_no_fg = self.service.backend(real_device_name, use_fractional_gates=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages were changed in this last commit, and I think the previous version was the correct one, please check.
| @@ -0,0 +1,5 @@ | |||
| While creating backend objects from cache when ``use_fractional_gates`` flag is used, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we start to create we still don't know if it's from cache. Also, the fix applies to both when the flag is used and when it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made changes for to the release note 👍
| @@ -0,0 +1,5 @@ | |||
| While creating backend objects from cache when ``use_fractional_gates`` flag is used, | |||
| `.QiskitRuntimeService` used to check for only ``rzz`` gates in the backend's basis. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for what? (check for whether to take from cache).
Summary
rxbut notrzz#2524