[unitaryHACK](#1) Cirq Support #20

harry-stark · 2021-05-31T05:18:51Z

These are the implementation of CirqBackend , CirqPlatform , CirqJob, CirqCircuit.

Fixes #1

harry-stark · 2021-05-31T05:31:44Z

qrand/platforms/cirq/circuit.py

-
-    def cx(self, control_qubit: int, target_qubit: int) -> None:
-        raise NotImplementedError(self.ERROR_MSG)
-


Official Implementation of CX, CS are WIP. Also some three-qubit gates too. We can wait until the CIRQ ships them officially.

harry-stark · 2021-05-31T05:35:20Z

qrand/platforms/cirq/backend.py

+    """
    @property
    def max_qubits(self) -> int:
-        raise NotImplementedError(self.ERROR_MSG)
+        return (
+
+        )
+    """


I couldn't figure out the maximum qubits part. Should we define this explicitly...??

I do not think that would be a good approach. We have to run the programs on physical backends/simulators, and they will have different capabilities.

Also, have you tested your code? It should not have let you instantiate this class without defining this property.

pedrorrivero

Here you have a first review @harry-stark.

I have a lot of doubts about the code. Do not know the motivation behind many design choices, and do not understand a number of other things. We will need to work hard on this to get it ready.

Importantly: you need to follow the contributing guidelines. You did not. You will need to reset your commit history, since your commit messages do not adhere to Conventinal commits. Before doing so, though, make sure to save a copy of this review outside of GitHub in case it gets deleted.

pedrorrivero · 2021-05-31T13:37:12Z

qrand/platforms/cirq/backend.py

@@ -19,23 +19,71 @@
 ## WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 ## See the License for the specific language governing permissions and
 ## limitations under the License.
-
+from cirq_google.engine import Engine 


I the dependency cirq, cirq_google, both, or are they the same? I am saying this to add list it appropriately. Also, did you include any extra depencies?

They are different packages and no other extra dependency was used.

pedrorrivero · 2021-05-31T13:40:28Z

qrand/platforms/cirq/backend.py

+
+
+


Generally speaking, adhere to the code style in other files. Here, remove unnecessary empty lines.

pedrorrivero · 2021-05-31T13:41:26Z

qrand/platforms/cirq/backend.py

+    """
+
+        Encapsulating Google Engine API with Quantum Backend 
+
+        Since API and Cirq is still in Alpha,Changes are expected
+
+        methods from Engine class:
+
+        create_program()->Wraps a Circuit for use with the Quantum Engine. i.e return engineprogram
+
+        run()->Runs the supplied Circuit via Quantum Engine. i.e return study.result    
+    """


This does not adhere to NumPy's docstring style. See the contributing guidelines.

pedrorrivero · 2021-05-31T13:45:15Z

qrand/platforms/cirq/backend.py

+                shots:int =1,
+                max_experiments:int=1


Does Cirq differentiate between shots and experiments? I thought they only used measurements. In any case, max_shots and max_experiments must be determined by the hardware you are connecting to, while the num_shots, num_experiments, or num_measurements has to be defined in CirqJob.

They use repetitions mainly.

pedrorrivero · 2021-05-31T13:51:38Z

qrand/platforms/cirq/backend.py

+    """
+    def __init__(
+                self, 
+                engine: Engine,


Why are you inheriting from Engine and also passing in and storing an Engine object as an attribute? Not necessarily wrong, but definitely not ideal, so just wondering the motivation behind it.

I thought maybe an Engine object had to be instantiated before gcloud authentication.But I think it's not necessary, will fix it in the fresh PR.

pedrorrivero · 2021-05-31T14:09:17Z

qrand/platforms/cirq/circuit.py

 from ..circuit import QuantumCircuit
-
+from ...helpers import validate_natural_number, validate_type


I believe that you are not using validate_type in this module.

pedrorrivero · 2021-05-31T14:10:32Z

qrand/platforms/cirq/circuit.py

+    def _validate_qubit_index(self, qubit_index: int) -> None:
+        if qubit_index >= self.num_qubits:
+            raise ValueError(
+                f"Qubit index out of range {qubit_index} >= {self.num_qubits}."
+            )
+
+
+


This has been added as a QuantumCircuit method. You can get rid of its declaration here. Keep using it though!

pedrorrivero · 2021-05-31T14:15:32Z

qrand/platforms/cirq/circuit.py

+    def createcircuit(self):
+        return CirqQuantumCircuit(self.moment_list)


It is better to inherit from CirqQuantumCircuit and apply the gates to itself: just as it is done in QiskitCircuit. This other technique should be avoided. Therefore you do not need the moment_list attribute.

Will do it.

pedrorrivero · 2021-05-31T14:18:40Z

qrand/platforms/cirq/platform.py

-##    _____  _____
-##   |  __ \|  __ \    AUTHOR: Pedro Rivero
-##   | |__) | |__) |   ---------------------------------
-##   |  ___/|  _  /    DATE: May 20, 2021
-##   | |    | | \ \    ---------------------------------
-##   |_|    |_|  \_\   https://github.com/pedrorrivero
+# _____  _____
+# |  __ \|  __ \    AUTHOR: Pedro Rivero
+# | |__) | |__) |   ---------------------------------
+# |  ___/|  _  /    DATE: May 20, 2021
+# | |    | | \ \    ---------------------------------
+# |_|    |_|  \_\   https://github.com/pedrorrivero
 ##

-## Copyright 2021 Pedro Rivero
+# Copyright 2021 Pedro Rivero
 ##
-## Licensed under the Apache License, Version 2.0 (the "License");
-## you may not use this file except in compliance with the License.
-## You may obtain a copy of the License at
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
 ##
-## http://www.apache.org/licenses/LICENSE-2.0
+# http://www.apache.org/licenses/LICENSE-2.0
 ##
-## Unless required by applicable law or agreed to in writing, software
-## distributed under the License is distributed on an "AS IS" BASIS,
-## WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-## See the License for the specific language governing permissions and
-## limitations under the License.
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.


Don't do this. You are making the style inconsistent across files.

pedrorrivero · 2021-05-31T14:23:20Z

qrand/platforms/cirq/platform.py

    def retrieve_backend(self) -> CirqBackend:
-        raise NotImplementedError(self.ERROR_MSG)
+        return CirqBackend(self._backend)


Where did you define self._backend?

I will fix it.

harry-stark · 2021-06-02T00:04:50Z

@pedrorrivero Well made a lot of rookie mistakes in this one. I will keep this a draft and open a parallel PR with less commit mess and better-tested code, adhering to the contributing guidelines. So we can refer to this review again until it is ready.

crazy4pi314 · 2021-06-08T05:29:16Z

Hey @harry-stark and @pedrorrivero! I wanted to let folks know I am extending the unitaryHACK deadline till the end of this week for PRs that were already in progress to give some time to iterate with maintainers. Let me know if you have questions, or if I can be of help in getting this issue closed!

harry-stark · 2021-06-08T18:30:40Z

@crazy4pi314 Thank you and that's some news. Will try my best to complete this before the weekend but only pain is that Google Qcs API is still private so very less info and working examples to look at. Will push some change soon.

harry-stark and others added 2 commits May 31, 2021 10:42

Added Cirq Support

6efd103

Merge branch 'pedrorrivero:dev' into dev

ed95a70

harry-stark commented May 31, 2021

View reviewed changes

pedrorrivero requested changes May 31, 2021

View reviewed changes

harry-stark marked this pull request as draft May 31, 2021 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[unitaryHACK](#1) Cirq Support #20

[unitaryHACK](#1) Cirq Support #20

harry-stark commented May 31, 2021

harry-stark May 31, 2021 •

edited

Loading

harry-stark May 31, 2021

pedrorrivero May 31, 2021

pedrorrivero left a comment •

edited

Loading

pedrorrivero May 31, 2021

harry-stark Jun 1, 2021

pedrorrivero May 31, 2021

pedrorrivero May 31, 2021

pedrorrivero May 31, 2021

harry-stark Jun 1, 2021

pedrorrivero May 31, 2021

harry-stark Jun 1, 2021

pedrorrivero May 31, 2021

pedrorrivero May 31, 2021

pedrorrivero May 31, 2021

harry-stark Jun 1, 2021

pedrorrivero May 31, 2021

pedrorrivero May 31, 2021

harry-stark Jun 2, 2021

harry-stark commented Jun 2, 2021

crazy4pi314 commented Jun 8, 2021

harry-stark commented Jun 8, 2021


		def cx(self, control_qubit: int, target_qubit: int) -> None:
		raise NotImplementedError(self.ERROR_MSG)

		from ..circuit import QuantumCircuit

		from ...helpers import validate_natural_number, validate_type

		def createcircuit(self):
		return CirqQuantumCircuit(self.moment_list)

[unitaryHACK](#1) Cirq Support #20

Are you sure you want to change the base?

[unitaryHACK](#1) Cirq Support #20

Conversation

harry-stark commented May 31, 2021

harry-stark May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrorrivero left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harry-stark commented Jun 2, 2021

crazy4pi314 commented Jun 8, 2021

harry-stark commented Jun 8, 2021

harry-stark May 31, 2021 •

edited

Loading

pedrorrivero left a comment •

edited

Loading