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 support for pennylane circuits #836

Merged
merged 38 commits into from
Oct 7, 2021
Merged

Add support for pennylane circuits #836

merged 38 commits into from
Oct 7, 2021

Conversation

rmlarose
Copy link
Contributor

Description

Checklist

Check off the following once complete (or if not applicable) after opening the PR. The PR will be reviewed once this checklist is complete and all tests are passing.

If some items remain, you can mark this a draft pull request.

Tips

  • If the validation check fails:

    1. Run make check-types (from the root directory of the repository) and fix any mypy errors.

    2. Run make check-style and fix any flake8 errors.

    3. Run make format to format your code with the black autoformatter.

    For more information, check the Mitiq style guidelines.

  • Write "Fixes #XYZ" in the description if this PR fixes Issue #XYZ.

@rmlarose rmlarose marked this pull request as draft July 23, 2021 15:17
Comment on lines 57 to 69
def test_to_from_pennylane():
circuit = cirq.testing.random_circuit(
qubits=4, n_moments=2, op_density=1, random_state=1
)

converted = from_pennylane(to_pennylane(circuit))

print(circuit)
print(converted)

assert cirq.testing.assert_allclose_up_to_global_phase(
cirq.unitary(converted), cirq.unitary(circuit), atol=1e-7,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Round-trip conversions not preserving qubit order. Output of this test:

      ┌──┐
0: ─────×────────
        │
1: ────×┼────H───
       ││
2: ────×┼────@───
        │    │
3: ─────×────@───
      └──┘
            ┌─────────┐
q_0: ───×────H────────────M('c_0')───
        │
q_1: ───×────@────────────M('c_1')───
             │
q_2: ───×────┼M('c_2')───────────────
        │    │
q_3: ───×────@────────────M('c_3')───
            └─────────┘

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @trbromley is it possible to reorder the wires of a quantum tape? (Long story motivating this question below.)

  1. Input Cirq circuit:
      ┌──┐
0: ─────×────────
        │
1: ────×┼────H───
       ││
2: ────×┼────@───
        │    │
3: ─────×────@───
      └──┘
  1. Resulting tape from tape = to_pennylane(circuit):
 1: ──╭SWAP───H──┤  
 2: ──╰SWAP──╭C──┤  
 0: ──╭SWAP──│───┤  
 3: ──╰SWAP──╰Z──┤  
  • Note this is the same as above with permuted qubit order. Namely (0, 1, 2, 3) -> (1, 2, 0, 3).
  1. print(tape.to_openqasm(rotations=True))
OPENQASM 2.0;
include "qelib1.inc";
qreg q[4];
creg c[4];
swap q[0],q[1];
h q[0];
swap q[2],q[3];
cz q[1],q[3];
measure q[0] -> c[0];
measure q[1] -> c[1];
measure q[2] -> c[2];
measure q[3] -> c[3];
  • Note the indices of the qubits in the QASM program do not correspond to the indices of the wires in the tape.
  • (Aside from main point about qubit order, but there are also added measurements that are not in the tape.)
  1. When converting back to Cirq from this QASM, the qubit order is the same as in the QASM program, so is permuted:
            ┌─────────┐
q_0: ───×────H────────────M('c_0')───
        │
q_1: ───×────@────────────M('c_1')───
             │
q_2: ───×────┼M('c_2')───────────────
        │    │
q_3: ───×────@────────────M('c_3')───
            └─────────┘

This can be fixed by reordering the wires in step 2, hence the question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @rmlarose! Good spot, thanks!

It looks like the tape in step 2 is ok, indeed you can specify the wire order when drawing the tape: tape.draw(wire_order=qml.wires.Wires([0, 1, 2, 3])).

However, it looks like an issue in tape.to_openqasm, since the wires in the QASM script are matched to the order specified by tape.wires = <Wires = [1, 2, 0, 3]>. A quick hack fix is to do:

tape.to_openqasm(wires=sorted(tape.wires))

Overall this looks like a bug/unexpected behaviour and I'll discuss with the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks! This is currently the main blocker for me. I'm fine with this suggested fix and the requirement that the tape's wires must be sortable. (Which isn't the case with wires labeled [0, "a"], for example.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding in the fix with pre-sorting the wires! Regarding the follow up comment on the PL-issue, is that a blocker for this PR? 🤔

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @rmlarose and sorry for being late to look at this!

The changes look great and thanks for making a start on the tests. My main question is whether we want rotations=True when converting to QASM from a tape. The positive is that we don't need to worry about the observable being measured, but a potential negative is that we might end up folding the rotation gates.

The current approach also loses the PL return type (e.g., measure expval, var, probs). The only way I can think to support that is with rotations=False and adding additional arguments to to_pennylane to persist the output measurement, but I can understand why it's preferable to have just the circuit as an input in to_pennylane.

It's also nontrivial to support custom wire labels (e.g., ["alice", "bob"] rather than integers). 🤔

The simplest solution is probably to restrict the PL circuits that can be mitigated. For example, we could raise an error if the input tape has custom wire labels or non-expval return types. If this makes sense, I'll be happy to make a PR from a fork.

mitiq/interface/mitiq_pennylane/conversions.py Outdated Show resolved Hide resolved
Args:
tape: Pennylane QuantumTape to convert to a Cirq circuit.
"""
return cirq_from_qasm(tape.to_openqasm(rotations=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the rotations=True part. The rotations diagonalize the measurement so we can just do it in the computational basis.

If we include the rotations at this stage, is it possible that the rotations get folded in he mitigation step? Which may or may not make sense, have to think about it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good point, thanks for bringing this up! Without regard to folding and just thinking about from_pennylane, it should include the rotations, otherwise the Cirq circuit - which only supports computational basis measurements - will not be equivalent.

However this makes it so that the Pennylane -> Cirq -> Pennylane sequence can start with non-computational basis measurements and end with computational basis measurements, which is a little strange. With hindsight we've realized conversions typically have problems like this, and have handled it in a "first do sloppy conversion, then clean it up" way. (Two examples of cleaning it up are adding back PyQuil declarations and keeping the same register structure in Qiskit.) This is a con of the design in my opinion, but the pro is that as long as you have to_whatever and from_whatever, even if it's (necessarily) a little sloppy, it makes it very easy to add support for whatever.

Anyway, you asked about folding the rotations: Since all other platforms measure in the Z basis we do fold rotations by default. There are options to not fold single-qubit gates, or certain types of gates, so whether or not rotations are there doesn't matter too much from the perspective of folding.

Copy link
Member

Choose a reason for hiding this comment

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

What about having the rotations argument in the function from_pennylane?

It seems that if we consider from_pennylane as a stand-alone function, the natural choice would be rotations=True .

On the other hand, if we also want to restore the full metadata of the tape (return type, observable, etc.) somewhere along the Mitiq worflow, it is probably easier if one can set rotations=False to decouple the bare circuit from the rest.

mitiq/interface/mitiq_pennylane/conversions.py Outdated Show resolved Hide resolved
Comment on lines 57 to 69
def test_to_from_pennylane():
circuit = cirq.testing.random_circuit(
qubits=4, n_moments=2, op_density=1, random_state=1
)

converted = from_pennylane(to_pennylane(circuit))

print(circuit)
print(converted)

assert cirq.testing.assert_allclose_up_to_global_phase(
cirq.unitary(converted), cirq.unitary(circuit), atol=1e-7,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @rmlarose! Good spot, thanks!

It looks like the tape in step 2 is ok, indeed you can specify the wire order when drawing the tape: tape.draw(wire_order=qml.wires.Wires([0, 1, 2, 3])).

However, it looks like an issue in tape.to_openqasm, since the wires in the QASM script are matched to the order specified by tape.wires = <Wires = [1, 2, 0, 3]>. A quick hack fix is to do:

tape.to_openqasm(wires=sorted(tape.wires))

Overall this looks like a bug/unexpected behaviour and I'll discuss with the team.

@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #836 (eb143d1) into master (6445c0b) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #836      +/-   ##
==========================================
+ Coverage   97.87%   97.96%   +0.09%     
==========================================
  Files          53       55       +2     
  Lines        2541     2605      +64     
==========================================
+ Hits         2487     2552      +65     
+ Misses         54       53       -1     
Impacted Files Coverage Δ
mitiq/_typing.py 100.00% <100.00%> (ø)
mitiq/interface/conversions.py 99.09% <100.00%> (+0.06%) ⬆️
mitiq/interface/mitiq_pennylane/__init__.py 100.00% <100.00%> (ø)
mitiq/interface/mitiq_pennylane/conversions.py 100.00% <100.00%> (ø)
mitiq/__init__.py 100.00% <0.00%> (ø)
mitiq/executor/executor.py 100.00% <0.00%> (ø)
mitiq/interface/mitiq_cirq/cirq_utils.py 100.00% <0.00%> (ø)
mitiq/zne/inference.py 97.42% <0.00%> (+0.07%) ⬆️
mitiq/zne/zne.py 96.66% <0.00%> (+0.11%) ⬆️
mitiq/observable/observable.py 100.00% <0.00%> (+1.35%) ⬆️

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 6445c0b...eb143d1. Read the comment docs.

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @rmlarose! This is looking great!

To confirm, the remaining item left to do is to add a check for the return type of the quantum tape in from_pennylane() to ensure that only returning expectation values (for now) is supported? I can help with this, just want to make sure you're on board.

I'd also like to do a few manual trial runs of mitigating PL tapes using this new functionality, to test for any edge cases I can think of. Is there anything remaining to be done in the frontend to get the PL-tape compatible with, for example, execute_with_zne or the higher level folding functionality?

dev_requirements.txt Outdated Show resolved Hide resolved
Comment on lines 57 to 69
def test_to_from_pennylane():
circuit = cirq.testing.random_circuit(
qubits=4, n_moments=2, op_density=1, random_state=1
)

converted = from_pennylane(to_pennylane(circuit))

print(circuit)
print(converted)

assert cirq.testing.assert_allclose_up_to_global_phase(
cirq.unitary(converted), cirq.unitary(circuit), atol=1e-7,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding in the fix with pre-sorting the wires! Regarding the follow up comment on the PL-issue, is that a blocker for this PR? 🤔

@rmlarose
Copy link
Contributor Author

Thanks @trbromley. I just pushed code adding Pennylane tapes to mitiq.QPROGRAM and the top-level to/from conversions. It's still a bit rough around the edges, and needs more tests.

Thanks for adding in the fix with pre-sorting the wires! Regarding the follow up comment on the PL-issue, is that a blocker for this PR? 🤔

There are some failing tests with PEC, so I would consider that blocking, yes.

To confirm, the remaining item left to do is to add a check for the return type of the quantum tape in from_pennylane() to ensure that only returning expectation values (for now) is supported? I can help with this, just want to make sure you're on board.

I'm actually not sure what you mean by this, sorry. Can you elaborate?

Is there anything remaining to be done in the frontend to get the PL-tape compatible with, for example, execute_with_zne or the higher level folding functionality?

Probably - An end-to-end example will tell. With the latest code we can kinda sorta fold gates in Pennylane tapes:

import numpy as np

import pennylane as qml
from mitiq import zne

with qml.tape.QuantumTape() as tape:
    qml.RX(np.pi / 2, wires=[0])
    qml.CNOT(wires=[0, 1])
    qml.expval(qml.PauliZ(wires=[0]))
    qml.expval(qml.PauliZ(wires=[1]))

print(tape.draw())

scaled_tape = zne.scaling.fold_all(tape, scale_factor=3)
print(scaled_tape.draw())
 0: ──RX(1.57)──╭C──┤ ⟨Z⟩ 
 1: ────────────╰X──┤ ⟨Z⟩ 


 0: ──RX(1.57)──RX(-1.57)──RX(1.57)──╭C──╭C──╭C──┤  
 1: ─────────────────────────────────╰X──╰X──╰X──┤  

Note the ⟨Z⟩s are removed in the scaled_tape. This would probably cause issues in an end-to-end example.

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @rmlarose!

To confirm, the remaining item left to do is to add a check for the return type of the quantum tape in from_pennylane() to ensure that only returning expectation values (for now) is supported? I can help with this, just want to make sure you're on board.

I'm actually not sure what you mean by this, sorry. Can you elaborate?

The suggestions I've left in this review should fix things. In PennyLane we can have other return types, for example:

with qml.tape.QuantumTape() as tape:
    qml.RX(0.2, wires=0)
    qml.RY(0.6, wires=1)
    qml.CNOT(wires=[0, 1])
    qml.var(qml.PauliY(0))

This would give the variance of PauliY. I think for this PR it's sufficient to support just expectation values.

There are some failing tests with PEC, so I would consider that blocking, yes.

This should be fixed in PennyLaneAI/pennylane#1559 - though this would not make it to a release version of PennyLane until September 🤔

@crazy4pi314 crazy4pi314 self-assigned this Aug 20, 2021
@nathanshammah
Copy link
Member

The validate CI fails seems fixable with black. The test/test fail seems do to an AssertionError .

@crazy4pi314
Copy link
Member

crazy4pi314 commented Sep 1, 2021

The validate CI fails seems fixable with black. The test/test fail seems do to an AssertionError .

@nathanshammah should have fixed this and merged in master to continue testing

crazy4pi314 and others added 4 commits September 8, 2021 10:45
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
…ne.py

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
@andreamari
Copy link
Member

andreamari commented Sep 20, 2021

Hello everyone, a quick update.

There was a remaining failing test due to a problem related to conversions of identity gates.
So I updated the failing test from an exact circuit comparison to a matrix comparison:

# TODO: test circuit equality after Identity operation will be added

The reason is that the Identity operation doesn't exist in PennyLane at the moment although there is a plan to add it (PennyLaneAI/pennylane#1632).
See, also PennyLaneAI/pennylane-qiskit#153.

Once those issues will be fixed, we can improve the test asserting gate equality instead of matrix equality.

There may be other subtle edge cases that I didn't notice. So feel free to add more tests.

A thing to decide before merging is what kind of tape we want to explicitly support. Do we want to raise an error each time a tape contains other things that are not gates (measurements, expvals, etc..)? This is a sub-optimal but probably safer solution.
If I am not wrong, @trbromley suggested something similar during the last PL community call.

@nathanshammah
Copy link
Member

nathanshammah commented Sep 23, 2021

About @andreamari's proposal:

A thing to decide before merging is what kind of tape we want to explicitly support. Do we want to raise an error each time a tape contains other things that are not gates (measurements, expvals, etc..)? This is a sub-optimal but probably safer solution.

I am in favor of this. We can extend functionality later. We could then add an example with basic integration. @trbromley what do you think?

@nathanshammah
Copy link
Member

I resolved the conflict updating the requirements to include the latest braket update on master branch.

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks everyone! This looks great.

I agree with @andreamari's suggestion to restrict the input tape to not have any measurements at this stage.

I have added a few suggestions, including an integration test that should give a deeper confirmation that the conversion is working as expected.

dev_requirements.txt Outdated Show resolved Hide resolved
Returns:
Mitiq circuit representation equivalent to the input QuantumTape.
"""
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

@WrathfulSpatula, agreed! We should probably check that the input wires are contiguous. As @rmlarose points out, the try/except is for the case of custom wire labels and I believe is still valid.

Comment on lines 65 to 69
# tape.to_openqasm always introduces measurements that we remove
measurements = _pop_measurements(output)
# Remove final empty moment if present
if measurements and output[-1] == Moment():
return output[:-1]
Copy link
Contributor

@trbromley trbromley Sep 29, 2021

Choose a reason for hiding this comment

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

Thanks @andreamari! An alternative approach might be to use tape.to_openqasm(rotations=True, wires=wires, measure_all=False), which we added in PennyLaneAI/pennylane#1559. This would need to be complemented by ensuring that tape.measurements is empty (as suggested).

The above would also require PL >= 0.18. So maybe your solution is perfect as is!

Copy link
Member

Choose a reason for hiding this comment

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

+1 for tape.to_openqasm(rotations=True, wires=wires, measure_all=False)

Copy link
Member

@andreamari andreamari Oct 1, 2021

Choose a reason for hiding this comment

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

Applied in this commit: eb143d1 . It works!

I also explicitly added pennylane>=0.18 in dev_requirements.txt to ensure previous versions of PL are updated in local environments.

mitiq/interface/mitiq_pennylane/conversions.py Outdated Show resolved Hide resolved
mitiq/interface/mitiq_pennylane/conversions.py Outdated Show resolved Hide resolved
mitiq/interface/mitiq_pennylane/conversions.py Outdated Show resolved Hide resolved
andreamari and others added 11 commits October 1, 2021 12:08
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
@andreamari
Copy link
Member

Thanks for all the great suggestions, @trbromley !

Side note: I am impressed that, after directly applying all your complex GitHub suggestions, tests passed out of the box! 😃

I am approving this PR, but I would wait for a double check by other Mitiq people before merging.

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @andreamari for the latest updates and to everyone for contributing to this! It looks good to go and I'm excited to have this integration 🚀

Regarding updating the documentation (e.g. adding to here), is that best done in a follow up PR? I'd think so, it's nice to get this PR across the line first.

@@ -3,6 +3,8 @@ qiskit-terra~=0.18.2
qiskit-aer~=0.8.2
qiskit-ibmq-provider~=0.16.0
pyquil~=2.28.0
pennylane-qiskit~=0.18.0
pennylane>=0.18
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you specify anywhere in the docs a minimum version for soft dependencies?

Copy link
Member

Choose a reason for hiding this comment

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

@crazy4pi314, @rmlarose, @nathanshammah what do you think?

In principle it would be nice to add info about minimum versions but there are some downsides:

  • It can easily become outdated.
  • In general, we actually don't know what are the minimum versions since we only test the specific versions pinned in dev_requirements.txt.

Copy link
Member

Choose a reason for hiding this comment

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

Andrea, I agree on the fact that it would be nice to show soft dependencies in the documentation.

It is also true as you say that with the dependabot in place, updating dependencies quite often, it would add an overhead to test the minimum versions working. I don't know what is a best practice here.

In my opinion we should keep the current dev requirements consistent with pinning to the minor (pennylane~=0.18); we could add to the docs a citation of the requirements and dev requirements, , e.g., scraping those files.

@crazy4pi314
Copy link
Member

I'll merge this now, and make sure we have an issue for user docs on this! Grats everyone 🎉

@andreamari
Copy link
Member

andreamari commented Oct 8, 2021

Even if tests were passing in this PR, after merging on master tests are failing for some pip installation problem.
I would propose clicking on the revert button above to undo the changes until we figure out how to fix the problem.
Problem solved in #976.

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.

6 participants