-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor quimb backend #13
Conversation
@scarrazza workflows are not running because we exceed our quota. |
However, locally on my Linux machine tests are now passing, and they are the exact same of #8 |
d88dd7a
to
bac9ad1
Compare
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!
Thanks, I'll wait for another review and then we can merge :) |
bac9ad1
to
662adfe
Compare
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, thanks.
After merging, pytest hit an error below. Seems the cause is that
E assert (0.01 * 1.6729983501136303) < 0.0031228289008140564 tests/test_qasm_quimb_backend.py:59: AssertionError |
This is an extra condition I added, not to leave times unused. It was a rough attempt, but I considered unlikely to fail. EDIT: I believe it might only fail on a machine with an available GPU, e.g. not mine, but that is an interesting information: why Qibo is so slow with |
Adopt Qibo QASM parser, i.e. construct the Quimb
Circuit
object directly from QiboCircuit
.The code inside
qibotn
is not any longer tailored for QFT. So, feel free to try with any other Qibo circuit, and report in case something is broken.(a known limitation is that Qibo gates have both
.qubits
attribute and `.target_qubits', but the latter is not used at present time; then, if qubits are renamed afterwards, w.r.t. the canonical naming, this is not propagated to Quimb - but I expect to be non-trivial to hit this corner case)QASM
Notice that the function still reads QASM, so in the tests a QASM string is still generated and passed to the function. But instead of writing a dedicated parser, the Qibo parser is used, and it generates a temporary Qibo circuit inside the
quimb
backend.So, at the moment there are two circuits: one in the tests, generating the QFT, that than is dump to QASM (still in tests), and the other inside the backend, loaded from the QASM.
If at some point we don't care any longer about accepting QASM input, we can always replace the input of
eval()
function (still lacking a better, but simple, name, given that I'm shadowing a built-in) directly with a Qibo circuit, and delegate the QASM parsing (i.e. the call toqibo.models.Circuit.from_qasm()
) to the user.Swaps
Before there was a dedicated handling for swaps. Given that Qibo is treating them as any other gate, at the moment I suppressed them everywhere, but in the
qibo.models.QFT
circuit generation.If you are aware of any explicit reason to treat them separately from the rest, please explain me and I will try to fix it accordingly.