[Pasqal updates] New qubit classes #3133

HGSilveri · 2020-07-13T10:42:11Z

This the first child-PR of #3126 .

Introduces new Pasqal qubit classes: ThreeDQubitand TwoDQubit (a subclass of ThreeDQubit)
These new classes are intended to replace the original ThreeDGridQubit, which will be deleted on the next PR (where other classes' dependencies will be resolved).
In comparison with ThreeDGridQubit, these new classes allow for arbitrary placement of qubits in 3D or 2D space, instead of restricting them to a grid. This more closely resembles the capabilities of Pasqal devices.

HGSilveri · 2020-07-13T11:00:25Z

Good news, @balopat! I managed to make a smaller first PR than previously anticipated by only adding the new qubit classes. ThreeDGridQubit had to be left here and will be deleted once PasqalDevice is updated, but I still think this will facilitate the review process. Thank you for all your help so far, I hope this helps!

balopat

This is looking really good @HGSilveri, it's the perfect size / scope.
Thank you for splitting it out!!!
I understand you based your design and comments based on GridQubit - which is a good starting point. I only have some nits and one or two questions - but I think we are basically good to go.

cirq/pasqal/pasqal_qubits_test.py

cirq/pasqal/pasqal_qubits.py

balopat · 2020-07-13T17:41:18Z

cirq/pasqal/pasqal_qubits.py

+        self.z = z
+
+    def _comparison_key(self):
+        return round(self.z, 9), round(self.y, 9), round(self.x, 9)


Why rounding the coordinates? And why to 9 digits?

I don't recall exactly what prompted this, but I'm assuming it was some machine precision error. We end up using a lot of irrational numbers from triangular lattice coordinates, so I'm assuming there was some false negative.
As for the rounding, I think it is arbitrary, it could have more digits. I'm happy to change it to a higher value.

Great - if you could recover that use case that would be useful - we should add it to the tests to protect this functionality. Otherwise someone might just remove this without failing tests.

Turns out rounding to 9 was excessive, I increased the number of digits to 15 and introduced a test to back that up.

cirq/pasqal/pasqal_qubits.py

HGSilveri · 2020-07-13T18:44:41Z

That's good to hear @balopat ! Thanks a lot for the quick review, I've addressed all your questions and nits.

balopat

Thank you! A couple more findings!

cirq/pasqal/pasqal_qubits.py

balopat · 2020-07-13T20:19:01Z

cirq/pasqal/pasqal_qubits.py

+        self.z = z
+
+    def _comparison_key(self):
+        return round(self.z, 9), round(self.y, 9), round(self.x, 9)


Great - if you could recover that use case that would be useful - we should add it to the tests to protect this functionality. Otherwise someone might just remove this without failing tests.

balopat · 2020-07-13T20:21:51Z

cirq/pasqal/pasqal_qubits.py

+        return cirq.protocols.obj_to_dict_helper(self, ['x', 'y', 'z'])
+
+    def __add__(self, other: Tuple[float, float, float]) -> 'ThreeDQubit':
+        if not (isinstance(other, tuple) and len(other) == 3 and


one last question: GridQubit does have the functionality to add/substract GridQubits together in here:

Cirq/cirq/devices/grid_qubit.py

Lines 76 to 82 in adc8964

if isinstance(other, _BaseGridQid):

if self.dimension != other.dimension:

raise TypeError(

"Can only add GridQids with identical dimension. "

f"Got {self.dimension} and {other.dimension}")

return self._with_row_col(row=self.row + other.row,

col=self.col + other.col)

Do you mind adding that with tests to be consistent?

Actually, I've been discussing with the rest of the team and we reached the conclusion that these dunder methods for addition and subtraction of qubits or tuples to qubits don't really make sense for our case.
They were first added because the ThreeDGridQubit class tried to mimic GridQubit and were just left there for no particular reason. As the ThreeDGridQubit class turned into ThreeDQubit, it made increasingly less sense to have them, so I am now advocating for their removal. However, before I do so, I would like to know, do you have any objection or counter-argument to this?

No counter argument / objection on my end - I prefer not having it at all then having it half-way implemented!

HGSilveri · 2020-07-15T16:47:30Z

Hi @balopat! As I commented above, we are dropping the addition and subtraction magic methods. I went ahead and pushed a new commit without them, but I can always revert it in case you have an objection. If you do, please let me know and I'll address it asap.

The other nits and requests have been addressed, so in case nothing else comes up, this one is hopefully good to go and we can move on to the next PR.

balopat

LGTM, thank you for all your work!

This the second child-PR of #3126 , following #3133 . - Generalizes `PasqalDevice`, removing ties to the physical placement of the qubits. - `PasqalDevice` now serves as the parent class of all future Pasqal devices, only enforcing constraints expected to exist on all future devices. - `PasqalDevice`can also be used as the device for submission of explicitly unconstrained circuits to Pasqal's devices. A new device class, `PasqalVirtualDevice`, will subclass the new `PasqalDevice` and recover the hardware constraints of the current `PasqalDevice` on a future PR.

This the first child-PR of quantumlib#3126 . - Introduces new Pasqal qubit classes: `ThreeDQubit`and `TwoDQubit` (a subclass of `ThreeDQubit`) - These new classes are intended to replace the original `ThreeDGridQubit`, which will be deleted on the next PR (where other classes' dependencies will be resolved). - In comparison with `ThreeDGridQubit`, these new classes allow for arbitrary placement of qubits in 3D or 2D space, instead of restricting them to a grid. This more closely resembles the capabilities of Pasqal devices.

This the second child-PR of quantumlib#3126 , following quantumlib#3133 . - Generalizes `PasqalDevice`, removing ties to the physical placement of the qubits. - `PasqalDevice` now serves as the parent class of all future Pasqal devices, only enforcing constraints expected to exist on all future devices. - `PasqalDevice`can also be used as the device for submission of explicitly unconstrained circuits to Pasqal's devices. A new device class, `PasqalVirtualDevice`, will subclass the new `PasqalDevice` and recover the hardware constraints of the current `PasqalDevice` on a future PR.

HGSilveri added 3 commits July 13, 2020 12:15

Introduced ThreeDQubit and TwoDQubit to pasqal module

1f0c80b

Added json serialization and tests for new Pasqal qubits

ae8da72

Comestic changes

16524a7

googlebot added the cla: yes Makes googlebot stop complaining. label Jul 13, 2020

Pylinting and formatting

6b5088c

balopat suggested changes Jul 13, 2020

View reviewed changes

Adressing some review nits

8621ed5

balopat suggested changes Jul 13, 2020

View reviewed changes

HGSilveri added 4 commits July 15, 2020 12:08

Increased rounding precision in ThreeDQubit _comparison_key

7349fcc

Fixed formatting error

89d806d

Merge branch 'master' into pasqal_new_qubits

e301330

Remove add/subtract dunder methods from ThreeDQubit

fa397c3

balopat approved these changes Jul 15, 2020

View reviewed changes

balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 15, 2020

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 15, 2020

Merge branch 'master' into pasqal_new_qubits

a5e1664

CirqBot merged commit 72808f3 into quantumlib:master Jul 15, 2020

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 15, 2020

HGSilveri mentioned this pull request Jul 16, 2020

[Pasqal updates] PasqalDevice class generalization #3141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pasqal updates] New qubit classes #3133

[Pasqal updates] New qubit classes #3133

HGSilveri commented Jul 13, 2020 •

edited

HGSilveri commented Jul 13, 2020

balopat left a comment

balopat Jul 13, 2020

HGSilveri Jul 13, 2020

balopat Jul 13, 2020

HGSilveri Jul 15, 2020

HGSilveri commented Jul 13, 2020

balopat left a comment

balopat Jul 13, 2020

balopat Jul 13, 2020

HGSilveri Jul 15, 2020 •

edited

balopat Jul 15, 2020

HGSilveri commented Jul 15, 2020

balopat left a comment

	if isinstance(other, _BaseGridQid):
	if self.dimension != other.dimension:
	raise TypeError(
	"Can only add GridQids with identical dimension. "
	f"Got {self.dimension} and {other.dimension}")
	return self._with_row_col(row=self.row + other.row,
	col=self.col + other.col)

[Pasqal updates] New qubit classes #3133

[Pasqal updates] New qubit classes #3133

Conversation

HGSilveri commented Jul 13, 2020 • edited

HGSilveri commented Jul 13, 2020

balopat left a comment

Choose a reason for hiding this comment

balopat Jul 13, 2020

Choose a reason for hiding this comment

HGSilveri Jul 13, 2020

Choose a reason for hiding this comment

balopat Jul 13, 2020

Choose a reason for hiding this comment

HGSilveri Jul 15, 2020

Choose a reason for hiding this comment

HGSilveri commented Jul 13, 2020

balopat left a comment

Choose a reason for hiding this comment

balopat Jul 13, 2020

Choose a reason for hiding this comment

balopat Jul 13, 2020

Choose a reason for hiding this comment

HGSilveri Jul 15, 2020 • edited

Choose a reason for hiding this comment

balopat Jul 15, 2020

Choose a reason for hiding this comment

HGSilveri commented Jul 15, 2020

balopat left a comment

Choose a reason for hiding this comment

HGSilveri commented Jul 13, 2020 •

edited

HGSilveri Jul 15, 2020 •

edited