-
Notifications
You must be signed in to change notification settings - Fork 32
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 a converter for QASM 2.0 to 3.0 #243
Conversation
I am not sure about the test fail as the unit test failure was present in files unaffected by this commit. |
@TheGupta2012 Yes, seems like a token is missing which would just be a GitHub actions permissions error. I'll take a look and lyk what I find! Should be an easy fix |
@TheGupta2012 Ok, try syncing with latest commit to main and see if that fixes it. If not can try one other approach |
Hey, thanks @ryanhill1! Seems like that was indeed the issue. |
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.
Your progress looks great so far! It seems like your code covers all of the conversions for gates from stdgates.inc
. As the next step, we'd also like to support conversions of gates from qelib1.inc
. Unlike in qasm2, you won't just be able to declare these gates with an "includes" statement. In qasm3, you will have to dynamically create a "custom" gate that implements each one, based on it's definition. Instead of hard-coding each case, you can maybe try to incorporate the above linked file, and read-in the gate definitions as needed as you parse through and convert the qasm2 string. Or, you take your own approach!
Below is some code that you can use to test your solution. To run it, you will have to first install the experimental qiskit qasm3 library:
pip install 'qiskit-terra[qasm3-import]'
from qiskit.qasm3 import loads
from qbraid.interface import circuits_allclose, random_circuit
from qbraid.interface.qbraid_qasm.tools import convert_to_qasm_3
for _ in range(100):
circuit = random_circuit("qiskit")
qasm2_str = circuit.qasm()
qasm3_str = convert_to_qasm_3(qasm2_str)
circuit_test = loads(qasm3_str)
assert circuits_allclose(circuit, circuit_test)
Hi @ryanhill1 , I incorporated your suggestion by building a I figured that since these are indeed present in the I didn't modify the I am currently experiencing a couple of bugs though. First, is the fact that expressions containing constants and identifiers are not being parsed correctly by the Second, is the failure of some of the random tests which I am yet to inspect more concretely. |
@TheGupta2012 Awesome, these updates are looking great so far! It seems like the qiskit team has already gotten back to you on the issue you raised: Qiskit/qiskit-qasm3-import#11 (comment). If they're slow pushing the changes on their end, maybe you can separate out the unit tests for the effected gates, and add a In terms of other random tests errors, if they continue to impact your progress, let me know and I can try to help debug. |
Hi @ryanhill1, I discovered one more bug in terra 😺. Its related to the Besides that, I have added the unit tests for custom gates and the decorated tests pertaining to the active bugs. An average of 80% of random tests are passing with some errors in qasm2 parsing. I saw that although the circuit structures (of the two circuits being compared) looked the same, the names of the gates may be the culprit for circuit inequality. Do you think converting circuits to unitary matrices and then comparing them would help? If you had time, could you also look at the failures? This was a script that I was using for the same - import os
import pytest
from qiskit.qasm3 import loads
from qbraid.interface import circuits_allclose, random_circuit
from qbraid.interface.qbraid_qasm.tools import convert_to_qasm_3
correct = 0
excepts = 0
i = 0
for _ in range(100):
try:
circuit = random_circuit("qiskit")
qasm2_str = circuit.qasm()
qasm3_str = convert_to_qasm_3(qasm2_str)
circuit_test = loads(qasm3_str)
eq = circuits_allclose(circuit, circuit_test)
if eq:
correct += 1
else:
print(f"Failed test {i}")
print(circuit.draw())
print(circuit_test.draw())
i += 1
except Exception as e:
print(e)
print("Qasm 2 string : ")
for line in qasm2_str.splitlines():
print(line)
print()
excepts += 1
print("Exceptions :", excepts)
print("Correct circuits : ", correct)
print("Incorrect : ", 100 - correct) |
@TheGupta2012 That's a great improvement on the random tests accuracy, good work! As for the remaining inequalities, if you look at the source code for the To verify, you could use the built-in qiskit methods to explicitly compare the unitaries as well: import numpy as np
from qiskit.quantum_info import Operator
circuit_unitary = Operator(circuit).data
circuit_test_unitary = Operator(circuit_test).data
np.allclose(circuit_unitary, circuit_test_unitary) although, this may actually lower the accuracy, because it does not account for global phase differences between the two circuits, while the Have you noticed any pattern as far as the types of circuits / gates that are failing? Another good test could be to restrict to |
Thanks for the suggestion @ryanhill1 ! I see ~ 95% of tests pass when testing for single qubit gates and depth = 1. I have raised an issue regarding this problem on the repository and I hope it gets resolved soon. |
@TheGupta2012 Ok, that sounds good. Same as before, if they're slow to make the change, then we can just note the inconsistency and skip the impacted test cases. |
Hi @ryanhill1 , the randomized test is now passing after excluding the problematic gates which we were discussing above. I have also added a note in the docstring of the test to reference the discussion about how the problematic gate list was generated. Let me know if any changes are required! |
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.
These changes look great! Just two final very small, nit-picky updates, and then everything should be good to go. Thanks for your hard work and diligence on this issue 🎉
Hi @ryanhill1 , so sorry for that CI failure, turns out I forgot to update the |
@TheGupta2012 No problem, small fix I got it! |
Changes:
Adds a new function
convert_to_qasm_3
inqbraid/interface/qbraid_qasm/tools.py
Details or comments:
Fixes #223
According to the OPENQASM specification, many constructs of v2.0 are compatible with v3.0. The changes are as follows -
qreg q[1];
toqubit[1] q;
andcreg c[1];
tobit[1] c;
measure q[i] -> c[j];
toc[j] = measure q[i];
. This also supports multi-qubit measurement (given the register sizes are the same)CX
gate is no longer a primitive operation. It is implemented using thectrl @
construct and its definition is readily supplied in the standard headerstdgates.inc
. This is equivalent to theqelib1.inc
header present in v2.0Some helper functions are used in code to change different constructs from v2.0 to v3.0. I have also included a semantic check using
qiskit
before converting to v3.0.One concern I have is about the keywords of v3.0 being used as identifiers in semantically valid v2.0 programs. Does that check need to be incorporated at this level?
Moreover, unit tests for each construct are added
Checklist before merge
General
I have read the CONTRIBUTING doc.
ALL new features must include unit test to the test directory, codecov will check for any missing test code exist.
Ensure that all the test passes.