Open khnikhil opened 4 months ago
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
View this failed invocation of the CLA check for more information.
For the most up to date status, view the checks section at the bottom of the pull request.
please refer to our dev guidline for how to work with out CIs and run them locally https://github.com/quantumlib/Cirq/blob/a9776d0f3613d675d68082aa620919a94658263c/docs/dev/development.md#setting-up-an-environment
Hi @NoureldinYosri , thanks for the quick reply and feedback. I fixed the issues with the automatic tests, and now just need to resolve any linting issues.
hi @NoureldinYosri , I delinted the new code following the instructions in the docs you referenced. It should be error free now.
hi @NoureldinYosri , would it be possible to run the checks again on my latest commit?
I fixed the formatting issues identified in the last set of tests (the delinter raised an error at a couple of unused packages) and it should be all good to go now!
hi @NoureldinYosri , gently bumping this - would it be possible to approve the workflow so the tests can be run on my new commit?
hi @NoureldinYosri , thank you very much for the comments. I've made the changes you suggested and have fixed the issues the automated tests caught - I ran the tests again on my local computer and they appear to be working.
I'm a bit confused to why my local checks haven't been catching these mistakes, as I've been following the dev guidelines you recommended (specifically, I've been running all the tests on all my changed files through the ./check/all [BASE_REVISION] [--only-changed-files] [--apply-format-changes]
command).
Thanks so much for the comments again.
hi @NoureldinYosri , I think I figured out the issue with running the automated tests on my computer. This latest commit fixes the issues with implementing @value.value_equality
instead of overwriting the __eq__
method as I had done before, and should pass all the tests now.
@khnikhil thanks for the PR ... I'm having second thoughts about this... I don't think we should have these gates as classes ... but they should be instances of classes. Both gates are Clifford so we should just have them as instances of the cirq.CliffordGate class https://github.com/quantumlib/Cirq/blob/543d9cd4f478805a759fc48cacccd1ea60a145c4/cirq-core/cirq/ops/clifford_gate.py#L358
this task just becomes creating the two objects
CXSWAP = CliffordGate.from_op_list([CNOT(*cirq.LineQubit.range(2)), SWAP(*cirq.LineQubit.range(2))], cirq.LineQubit.range(2))
CZSWAP = CliffordGate.from_op_list([CZ(*cirq.LineQubit.range(2)), SWAP(*cirq.LineQubit.range(2))], cirq.LineQubit.range(2))
so to complete this task just
CXSWAP = CliffordGate.from_op_list([CNOT(*cirq.LineQubit.range(2)), SWAP(*cirq.LineQubit.range(2))], cirq.LineQubit.range(2)).rename('CXSWAP')
``
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 97.81%. Comparing base (
543d9cd
) to head (e2d1ec9
). Report is 61 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi @NoureldinYosri , it seems that implementing the CXSWAP
and CZSWAP
gates in this way inevitably leads to a circular import (caused by calling cirq.LineQubit
). Basically, it seems that calling any object in Cirq/cirq-core/cirq/devices
from a file in Cirq/cirq-core/cirq/ops
leads to a circular import.
I tried circumnavigating the issue by using lazy imports (see attached screenshot showing similar code in other files in Cirq/cirq-core/cirq/ops
).
However, this too failed when I ran the tests, with the following error:
AttributeError: partially initialized module 'cirq.ops' has no attribute 'Qid' (most likely due to a circular import)
(see my code below)
It seems that even with a lazy import, using cirq.LineQubit
in this way is problematic.
Unless you have an idea of how to solve this, I believe that the original method of making a new class for the gates is the best way to move forward.
Also, I believe the only failed test from my last commit was in files that my code didn't alter at all (specifically, cirq-google/cirq_google/cloud/quantum_v1alpha1/services/quantum_engine_service/
)
Unless you have an idea of how to solve this
create the objects locally on your machine and get their cliffordtableau and then use from_clifford_tableau
method in your PR so
locally do
gate = CliffordGate.from_op_list(...)
print(repr(gate.clifford_tableau))
then in the PR add just the two lines
CXSWAP = CliffordGate.from_clifford_tableau(the CXSWAP Clifford tableau you computed locally).rename('CXSWAP')
CZSWAP = CliffordGate.from_clifford_tableau(the CZSWAP Clifford tableau you computed locally).rename('CZSWAP')
Where rename
is a new method you should add to override the representation of gates in circuit diagrams
I believe that the original method of making a new class for the gates is the best way to move forward.
nope, that will introduce unncessary code that will turn into code debt
@NoureldinYosri thanks for the quick response. I'm a bit confused - how exactly would I create the objects locally and then reference it in the PR?
Also, one other issue I anticipate running into is creating the corresponding json file in cirq-core/cirq/protocols/json_test_files
. I looked at the CX.json
file for reference, which is the following:
{ "cirq_type": "CXPowGate", "exponent": 1, "global_shift": 0.0 }
Since I'm building the gate using the .from_clifford_tableau
function, what would the appropriate representation be in the JSON file? My instinct would be to set it to
{ "cirq_type": "CliffordGate" }
since it doesn't take any other arguments initially. Is this correct?
how exactly would I create the objects locally and then reference it in the PR?
you won't. you will create a new object ... CliffordTableau is a representation of clifford gates as three matricies ... so you will just copy thier values when you construct the objects
Also, one other issue I anticipate running into is creating the corresponding json file
you won't. the json issue was because you were creating new classes ... but now you are creating objects.
all you need to do is add the two lines that create the two objects in clifford_gate.py, create the rename method and 2 tests for each gate the first checks its unitary and the second checks its circuit representation
@khnikhil - FYI the deadline for finishing in-flight PRs for the unitary hack has been extended to June 26th.
Would you be interested in giving this one more try following Nour's tips above (ie, https://github.com/quantumlib/Cirq/pull/6620#issuecomment-2150758120 and https://github.com/quantumlib/Cirq/pull/6620#issuecomment-2161958247)?
Fixes #6424 for UnitaryHack 2024.
Specifically, I added two gate classes,
CZSWAPGate
andCZSWAPGate
incirq-core.cirq.ops
which implement the gates (see below screenshot for example).