Skip to content
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 pasqal device #31

Merged
merged 11 commits into from
Jul 8, 2020
Merged

Add pasqal device #31

merged 11 commits into from
Jul 8, 2020

Conversation

co9olguy
Copy link
Member

@co9olguy co9olguy commented Jun 19, 2020

Adds a new device "cirq.pasqal", which uses the cirq.pasqal.PasqalDevice simulator as the execution backend. Cirq should be installed from the following fork: https://github.com/lhenriet/Cirq

Notes

  • Pasqal devices require ThreeDGridQubits, which are qubits which have positions in three dimensions.
  • Based on reading the plugin code, there are a number of places where an implicit assumption is made that the qubits are LineQubit:
    • in initialization of cirq_device.py
    • in _apply_basis_state in simulator_device.py
    • in _apply_qubit_state_vector in simulator_device.py
    • in apply in simulator_device.py
  • It should be relatively straightforward to allow other qubit types (e.g., GridQubit or the ThreeDGridQubits needed for the PasqalDevice, but it would require some small updates to the plugin to support more general cases than LineQubit. In particular, I identified that wherever len is called on the qubits, it will fail unless the qubits are LineQubits.
  • Fortunately, Cirq provides total ordering methods, so we can unambiguously assign every qubit in a grid a reproducible integer number, which allows us to map from PL's linear wire naming to the other schemes
  • PasqalDevice also requires an initialization argument control_radius. Two qubits in the grid can undergo two-qubit gates iff they are within control_radius of one another

With the addition of better handling for different qubit arrangements, I think we can relatively easily add the PasqalDevice.

One option for the future is to perhaps expose the positions of the qubits in 3D space as a variable that Pennylane can tune and optimize, but I think it might be tricky at the moment, since it would require updating the qubits after the device is initialized (and I'm not exactly sure whether Cirq/Pasqal supports non-static arrangements yet)

@josh146
Copy link
Member

josh146 commented Jun 30, 2020

@co9olguy, I have a couple of questions you might be able to answer on this PR:

but it would require some small updates to the plugin to support more general cases than LineQubit. In particular, I identified that wherever len is called on the qubits, it will fail unless the qubits are LineQubits.

From what I can tell, qubits accepts a list of ThreeDGridQubits, so length will always be well defined? e.g.,

qubits = [
    pasqal.ThreeDGridQubit(0, 0, 0),
    pasqal.ThreeDGridQubit(0, 1, 0),
    pasqal.ThreeDGridQubit(1, 0, 0),
    pasqal.ThreeDGridQubit(1, 1, 0)
]

dev = qml.device("cirq.pasqal", wires=4, shots=1000, analytic=True, qubits=qubits, control_radius=0.1)

@qml.qnode(dev, interface="tf")
def circuit(params):
    qml.templates.BasicEntanglerLayers(params, wires=[0, 1, 2, 3])
    return qml.expval(qml.PauliZ(0) @ qml.PauliX(2))

works fine for me without the isinstance(qubit, LineQubit) and ... validation.

Fortunately, Cirq provides total ordering methods, so we can unambiguously assign every qubit in a grid a reproducible integer number, which allows us to map from PL's linear wire naming to the other schemes

This one follows from above; it seems that the len(qubits) works for sequences of ThreeDGridQubits, so an internal mapping shouldn't be needed?

One option for the future is to perhaps expose the positions of the qubits in 3D space as a variable that Pennylane can tune and optimize, but I think it might be tricky at the moment, since it would require updating the qubits after the device is initialized

The main hurdle I can see though is that ThreeDGridQubit accepts only integers for the coordinates, not floats, so will not be trainable for now. The other issue is (as you mention) the parameters for the qubit position needs to somehow be passable as a QNode argument so that they pass through the ML interface.

This could be done by creating 'pseudo' quantum operation, that the cirq.pasqal device knows how to implement:

@qml.qnode(dev, interface="tf")
def circuit(params, x, y, z):
    pennylane_cirq.move_qubit(x, y, z, wires=0)
    qml.templates.BasicEntanglerLayers(params, wires=[0, 1, 2, 3])
    return qml.expval(qml.PauliZ(0) @ qml.PauliX(2))

@co9olguy
Copy link
Member Author

co9olguy commented Jul 3, 2020

The main hurdle I can see though is that ThreeDGridQubit accepts only integers for the coordinates, not floats

Where did you see this?

@co9olguy
Copy link
Member Author

co9olguy commented Jul 3, 2020

From what I can tell, qubits accepts a list of ThreeDGridQubits, so length will always be well defined? e.g.,

qubits = [
    pasqal.ThreeDGridQubit(0, 0, 0),
    pasqal.ThreeDGridQubit(0, 1, 0),
    pasqal.ThreeDGridQubit(1, 0, 0),
    pasqal.ThreeDGridQubit(1, 1, 0)
]

dev = qml.device("cirq.pasqal", wires=4, shots=1000, analytic=True, qubits=qubits, control_radius=0.1)

@qml.qnode(dev, interface="tf")
def circuit(params):
    qml.templates.BasicEntanglerLayers(params, wires=[0, 1, 2, 3])
    return qml.expval(qml.PauliZ(0) @ qml.PauliX(2))

works fine for me without the isinstance(qubit, LineQubit) and ... validation.

I think what happened (forgive me, been a couple weeks) is I eventually figured it out (you need to pass a list of qubits rather than a single qubit object), but I didn't cross that out of my working notes above before posting here

@josh146 did you actually evaluate the QNode? 😀

@co9olguy co9olguy closed this Jul 3, 2020
@co9olguy co9olguy reopened this Jul 3, 2020
@josh146
Copy link
Member

josh146 commented Jul 4, 2020

Where did you see this?

The ThreeDGridQubit accepts only integers for the coordinates, as indicated by type hints and docstrings:

https://github.com/lhenriet/Cirq/blob/125722761ca56a641db7e15c89303ed55f076441/cirq/pasqal/pasqal_qubits.py#L20

There's no specific validation stopping us from initializing the grid with floats instead, but it might not work end-to-end, since it looks like integers are assumed? It gets confusing though, since the PasqalDevice class sometimes assumes that these values are floats.

@josh146 did you actually evaluate the QNode? 😀

Almost! Everything seemed to work (applying etc.) until device.execute() was called. It fails with PasqalDevice has no simulate method. I couldn't quite work out how the Pasqaldevice was to be executed

@co9olguy
Copy link
Member Author

co9olguy commented Jul 7, 2020

The ThreeDGridQubit accepts only integers for the coordinates, as indicated by type hints and docstrings:

https://github.com/lhenriet/Cirq/blob/125722761ca56a641db7e15c89303ed55f076441/cirq/pasqal/pasqal_qubits.py#L20

Interesting, this looks like a recent change (less than 3 weeks ago). Wonder why? 🤔

Almost! Everything seemed to work (applying etc.) until device.execute() was called. It fails with PasqalDevice has no simulate method. I couldn't quite work out how the Pasqaldevice was to be executed

Got everything to work now. I just needed to take some time to understand better how Cirq implements things (e.g., Simulator and Device classes have completely different roles)

@co9olguy
Copy link
Member Author

co9olguy commented Jul 7, 2020

@josh146 looks like some of the tests are failing because of that cirq versioning bug 😦. Otherwise, this is ready for review. Since it depends on a non-official branch for the moment, I've set up a holding branch. In the end, the implementation didn't need to be very complex at all

@co9olguy co9olguy changed the base branch from master to pasqal-holding July 7, 2020 19:16
@co9olguy co9olguy requested a review from josh146 July 7, 2020 19:16
@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #31 into pasqal-holding will not change coverage.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           pasqal-holding       #31   +/-   ##
================================================
  Coverage          100.00%   100.00%           
================================================
  Files                   6         7    +1     
  Lines                 222       236   +14     
================================================
+ Hits                  222       236   +14     
Impacted Files Coverage Δ
pennylane_cirq/cirq_device.py 100.00% <ø> (ø)
pennylane_cirq/__init__.py 100.00% <100.00%> (ø)
pennylane_cirq/pasqal_device.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed19a17...d105451. Read the comment docs.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @co9olguy! Ran it locally with TensorFlow, and it works well (if but a little slow):

qubits = [
    pasqal.ThreeDGridQubit(0, 0, 0),
    pasqal.ThreeDGridQubit(0, 1, 0),
    pasqal.ThreeDGridQubit(1, 0, 0),
    pasqal.ThreeDGridQubit(1, 1, 0)
]

dev = qml.device("cirq.pasqal", wires=4, qubits=qubits, control_radius=0.1)

@qml.qnode(dev, interface="tf")
def circuit(params):
    qml.templates.BasicEntanglerLayers(params, wires=[0, 1, 2, 3])
    return qml.expval(qml.PauliZ(0) @ qml.PauliX(2))

params = qml.init.basic_entangler_layers_normal(n_layers=2, n_wires=4)
params = tf.Variable(params)

print(circuit(params))

Only questions/concerns I have are:

  • How we want to proceed with changing the Cirq requirements? Changing the requirement to the fork is required for development/testing, but it could go out of sync with Cirq master. Do we know of a plan to merge the fork back into Cirq master?

  • Documentation: we should add a page doc/devices/pasqal.rst, and have a card that links to this in doc/index.rst. However, the documentation pennylane-cirq.readthedocs.io is currently pinned to latest --- we should change this to stable so that it doesn't appear to users that navigate to the plugin docs.

requirements.txt Outdated Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
@@ -1,3 +1,4 @@
pennylane>=0.9
cirq>=0.7
google-api-core[grpc]<=1.14.0
git+https://github.com/lhenriet/Cirq.git
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we want to deal with this requirement when merging into master? The issue I see is that this fork has diverged from Cirq master (it hasn't been kept up to date).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this in a separate branch, and undocumented, until that fork is successfully merged in. I'll try to keep abreast of when that might happen. In the meantime, we can continue our next steps (developing a tutorial) using the holding branch

pennylane_cirq/pasqal_device.py Outdated Show resolved Hide resolved
pennylane_cirq/pasqal_device.py Outdated Show resolved Hide resolved
def __init__(self, wires, shots=1000, analytic=True, qubits=None, control_radius=1.0):

if not qubits:
qubits = [pasqal.ThreeDGridQubit(wire, 0, 0) for wire in range(wires)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another future option is to make use of the ThreeDGridQubit static methods, such as parallelep and rect to construct the array of qubits (https://github.com/lhenriet/Cirq/blob/125722761ca56a641db7e15c89303ed55f076441/cirq/pasqal/pasqal_qubits.py#L102)

pennylane_cirq/pasqal_device.py Show resolved Hide resolved
pennylane_cirq/pasqal_device.py Outdated Show resolved Hide resolved
@co9olguy co9olguy marked this pull request as ready for review July 8, 2020 17:11
@co9olguy co9olguy merged commit ac41fb2 into pasqal-holding Jul 8, 2020
@co9olguy co9olguy deleted the pasqal branch July 8, 2020 17:11
@co9olguy co9olguy changed the title [WIP] Add pasqal device Add pasqal device Jul 24, 2020
co9olguy added a commit that referenced this pull request Oct 8, 2020
* [WIP] Add pasqal device (#31)

* adding basic pasqal device

* using base simulator and removing qubit worries

* adding tests

* changes

* merge master and fix travis issue

* linting

* change version

* Update pennylane_cirq/pasqal_device.py

Co-authored-by: Josh Izaac <[email protected]>

* Update pennylane_cirq/pasqal_device.py

Co-authored-by: Josh Izaac <[email protected]>

* Update pennylane_cirq/pasqal_device.py

Co-authored-by: Josh Izaac <[email protected]>

Co-authored-by: Josh Izaac <[email protected]>

* Update branch (#33)

* Update requirements.txt

* Update requirements.txt

* Update requirements.txt

* Update pasqal_device.py

* Update test_pasqal_device.py

Co-authored-by: Josh Izaac <[email protected]>

* change coords

* fix

* proper attaching of pasqal device

* debugging

* cleanup and explanation

* updates for failing tests

* updating requirements to master branch of cirq

* ordering test upgrades

* fixing more edge cases

* Update pennylane_cirq/cirq_device.py

* removing print

* make identity gates local

* Update pennylane_cirq/cirq_device.py

Co-authored-by: Josh Izaac <[email protected]>

* imports

* black

* Update format.yml

* Update pennylane_cirq/cirq_device.py

Co-authored-by: Josh Izaac <[email protected]>

* blacker

* fixes

* docs

* more dox

* the

* Update tests.yml

* Update doc/devices/pasqal.rst

Co-authored-by: Josh Izaac <[email protected]>

* Update tests.yml

* Update tests.yml

* Update tests.yml

Co-authored-by: Josh Izaac <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants