-
Notifications
You must be signed in to change notification settings - Fork 69
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
Compatibility with 1.0 for devices #493
Conversation
[sc-56263] |
[sc-56126] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #493 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 552 557 +5
=========================================
+ Hits 552 557 +5 ☔ View full report in Codecov by Sentry. |
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.
One comment/question, otherwise ready to approve.
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 @lillian542!
wires (int or Iterable[Number, str]]): Number of subsystems represented by the device, | ||
or iterable that contains unique labels for the subsystems as numbers (i.e., ``[-1, 0, 2]``) | ||
or strings (``['aux_wire', 'q1', 'q2']``). | ||
backend (str): the desired backend |
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.
What would the other option be here?
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.
Currently there is no other option here, but they add/change backends sometimes. Our default format for the device init
functions so far has been that we don't worry about that - users can check backend options and pass what they want from the list of available backends. The list of available backends is in this case is ["basic_simulator"]
. Maybe this should move to kwargs
though? It's just here to match the other device signatures right now.
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.
Right, I'd be a bit confused about the backend option. If I put backend='aer_simulator'
, things wouldn't work?
I'm happy to leave this in for now though, as the precedent is already set in other devices.
pennylane_qiskit/basic_aer.py
Outdated
Keyword Args: | ||
name (str): The name of the circuit. Default ``'circuit'``. | ||
compile_backend (BaseBackend): The backend used for compilation. If you wish | ||
to simulate a device compliant circuit, you can specify a backend here. |
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.
Are these the only two kwargs
one might want to use?
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.
No, but we generally haven't documented allowed kwargs, because they are really determined by Qiskit, are backend-dependent, and we just pass them through. We don't have a complete list of options. I'll go over them and make sure we aren't missing any PennyLane specific ones though.
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.
I can't come up with any other PennyLane kwargs we would want to include here
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.
🤔 I'm thinking we remove these two, since I don't understand how or why I'd want to use them 😆
Co-authored-by: Thomas R. Bromley <[email protected]>
Co-authored-by: Thomas R. Bromley <[email protected]>
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.
Looks good
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 @lillian542! Just a few more comments, but overall the PR looks good enough to merge.
By default, tests on the ``ibmq`` device run with ``ibmq_qasm_simulator`` backend. At | ||
the time of writing this means that the test are "free". |
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.
Not sure what we will do here regarding this announcement 🤔 We might need to migrate to using the local integration tests.
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.
Are these remote tests being run in the plugin test matrix?
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.
They are running in the plugin matrix. And yes, we've started seeing warnings about that when running the tests.
Probably we will want to change our default backend for devices that currently use the ibmq_qasm_simulator
to be the aer_simulator
instead, since that's one of their recommended options in their migration documentation.
For our test, I think almost everything using the ibmq_qasm_simulator
can also just be replaced with the aer_simulator
for now. The fake providers and backends sound nice long-term, but just using aer_simulator
should be pretty quick fix to keep our tests working in the meantime.
wires (int or Iterable[Number, str]]): Number of subsystems represented by the device, | ||
or iterable that contains unique labels for the subsystems as numbers (i.e., ``[-1, 0, 2]``) | ||
or strings (``['aux_wire', 'q1', 'q2']``). | ||
backend (str): the desired backend |
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.
Right, I'd be a bit confused about the backend option. If I put backend='aer_simulator'
, things wouldn't work?
I'm happy to leave this in for now though, as the precedent is already set in other devices.
pennylane_qiskit/basic_aer.py
Outdated
Keyword Args: | ||
name (str): The name of the circuit. Default ``'circuit'``. | ||
compile_backend (BaseBackend): The backend used for compilation. If you wish | ||
to simulate a device compliant circuit, you can specify a backend here. |
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.
🤔 I'm thinking we remove these two, since I don't understand how or why I'd want to use them 😆
* update to use requirements-ci and requirements-ci-legacy * unpin qiskit-ibm-runtime * add explicit qiskit-ibm-runtime requirement * unpin in upload yaml * update changelog
Finishing the work started before the 0.35 release of getting the devices up to date and tested with Qiskit 1.0. We continue also running the tests with Qiskit 0.45.3, to check backward compatibility.
There are a few small tweaks to source code to get it compatible, but most of that got done before last release.
This PR is mostly
BasicAer
withBasicSimulator
going forward