-
-
Notifications
You must be signed in to change notification settings - Fork 162
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 support for qibo circuits #2102
Conversation
Identity insertion did no support circuit types different from Cirq, not it does.
I don't know if it helps, but on Stack Overflow there is discussion on it. |
Co-authored-by: Misty Wahl <82074193+Misty-W@users.noreply.github.com>
The build / validate is complaining about a mypy error. This error is coming from code in mitiq/rem/inverse_confusion_matrix.py which has NOT been modified from this PR. The dependabot PR for moving from OpenFermion 1.6.0 to 1.6.1 shows the same error: |
@francescsabater this issue is fixed on |
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.
LGTM @francescsabater, thank you for staying with it through all the back and forth!
@natestemen I think the changes you requested are addressed but wanted to give you a chance to take a look before merging.
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.
Awesome work Francesc. Apologies for the slow turnaround time on this, but I think it's best that we got it done the "right" way. Thank you for your patience.
Just left a few minor things/questions, but good to go once those are addressed/answered!
if gate.is_controlled_by: | ||
raise UnsupportedQiboCircuitError( | ||
"OpenQASM does not support multi-controlled gates." | ||
) |
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.
@francescsabater can you comment on why this error here is needed? From the looks of the link Misty provided it looks like multi-controlled gates should be possible. Whether they are actually implemented is another story, but it would be good to know what the situation is.
# Remove problematic comment lines in the qasm code | ||
qasm = "\n".join( | ||
[ | ||
line | ||
for line in qasm.split("\n") | ||
if not line.strip().startswith("//") | ||
] | ||
) | ||
# Remove problematic spaces in gate arguments | ||
qasm = "\n".join([line.replace(", ", ",") for line in qasm.split("\n")]) | ||
# Remove in line problematic comments | ||
lines = qasm.split("\n") | ||
clean_lines = [] | ||
for line in lines: |
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 think we can avoid converting between strings and lists a few times here by keeping things as a list after the first conversion. WDYT? (code not tested and formatting may be off!)
# Remove problematic comment lines in the qasm code | |
qasm = "\n".join( | |
[ | |
line | |
for line in qasm.split("\n") | |
if not line.strip().startswith("//") | |
] | |
) | |
# Remove problematic spaces in gate arguments | |
qasm = "\n".join([line.replace(", ", ",") for line in qasm.split("\n")]) | |
# Remove in line problematic comments | |
lines = qasm.split("\n") | |
clean_lines = [] | |
for line in lines: | |
# Remove problematic comment lines in the qasm code | |
qasm = [ | |
line.replace(", ", ",") # remove spaces in gate arguments | |
for line in qasm.split("\n") | |
if not line.strip().startswith("//") | |
] | |
clean_lines = [] | |
for line in qasm: |
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 thing with multi-controlled gates is that in the QiboCircuit.to_qasm()
function from Qibo, if the gate is controlled by a similar error is raised. They do not comment much on why they raise the error though.
https://github.com/qiboteam/qibo/blob/fb55eee2eca731a10fa97a6d2195ee1fa65f5b9a/src/qibo/models/circuit.py#L1160
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've managed to avoid some of the unnecessary back-and-forth conversions between lists and strings, but I had to edit the suggested code a bit since mypy was complaining about the inconsistent type of qasm
.
@francescsabater you may merge when you’re ready, or if the button is grayed out, let me know and I’ll do it. |
Great to have this in, thanks for the persistence @francescsabater and all who helped in the review, @Misty-W @natestemen @bdg221. |
Add support for Qibo circuits. Create the needed functions for the conversions between Qibo and Cirq and their respective tests. Update the documentation. Also, fix a bug in identity_insertion that did not allow to implement identity scaling with non-cirq circuits:
Add Qibo to dev requirments
Add to_qibo and from_qibo to conversions.
Add Qibo to QPROGRAM
Create init file in mitiq_qibo.
Create Qibo conversions file.
Create tests file for Qibo conversions.
Update test_pec.
Update frontends-backends.md
Add example qibo-noisy-simulation
Solve bug in identity insertion.
I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.
Authored by Francesc Sabater francescsabater11@gmail.com.
This contribution is part of the internship of the Master In Quantum Science and Technology of Barcelona carried out at the Barcelona Supercomputing Center under the supervision of Alba Cervera-Lierta.