-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add IQM devices #329
base: main
Are you sure you want to change the base?
Add IQM devices #329
Conversation
c30971f
to
7c1eafa
Compare
src/mqt/bench/tket_helper.py
Outdated
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 not entirely sure if this is the best solution. The r-gate is not available in Tket.
As far as I can tell any r-gate can be implemented by a single u3-gate but not vice versa(?).
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 the current native gates really the correct ones? See above for contradicting fact sheets from IQM.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #329 +/- ##
=======================================
+ Coverage 93.0% 93.2% +0.1%
=======================================
Files 47 48 +1
Lines 2437 2493 +56
=======================================
+ Hits 2268 2324 +56
Misses 169 169
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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.
The only major comment is regarding the native gate set. Depending on your answer to that we can discuss the pytket handling.
{ | ||
"name": "iqm_apollo", | ||
"num_qubits": 20, | ||
"basis_gates": ["r", "cz", "measure", "barrier"], |
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.
According to this documentation (https://iqm-finland.github.io/cirq-on-iqm/api/iqm.cirq_iqm.devices.apollo.Apollo.html), the native gate set is different.
{ | ||
"name": "iqm_adonis", | ||
"num_qubits": 5, | ||
"basis_gates": ["r", "cz", "measure", "barrier"], |
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.
According to this documentation (https://iqm-finland.github.io/cirq-on-iqm/api/iqm.cirq_iqm.devices.adonis.Adonis.html#iqm.cirq_iqm.devices.adonis.Adonis), the native gate set is different.
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.
Definitions for these gates can be found in the Cirq documentation:
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.
Well, that is curious. I got the structure for the calibration data from this source. Also, I have rechecked with the LRZ lab team, and they have confirmed the native gate set is, in fact, "r" and "cz".
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 doublechecked now with the hardware guys and they reassured me "r" and "cz" are the correct native gates.
This is consistent with the iqm-client and qiskit-on-iqm. I also found this mapping for the cirq-on-iqm.
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 for double checking! I suspected as much. The R gate is just the phased x-pow gate and the other two gates are special cases where certain angles are set to some fixed values. So the R gate can represent all the possible gates.
What I would assume though (could be wrong), is that the simpler gates typically have better fidelities and are faster. So, in the future, it might make sense to differentiate between them, just because they are calibrated differently.
For now, the R gate should be fine and the appropriate definition for TKET is given by the special paramterization of the U gate.
src/mqt/bench/tket_helper.py
Outdated
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 the current native gates really the correct ones? See above for contradicting fact sheets from IQM.
@@ -0,0 +1,99 @@ | |||
"""Test the IQMProvider class and the IonQ devices.""" |
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 am wondering whether we could in general improve the provider class tests. Currently, often only the type itself is tested while the type is already clear from the mypy type checking. Could we instead test for actual values or value ranges?
Added configuration for IQM: