pyQuil support in Open-Controls #14

rajibchakravorty · 2019-07-02T03:42:06Z

Method to convert DDS to pyquil.Program added within pyquil sub-package.

# This is the 1st commit message: starting pyquil support # This is the commit message #1: pyquil support added # This is the commit message #2: first version of pyquil notebook added # This is the commit message #3: pyquil dependency added to setup # This is the commit message #4: trying gcc install for pyquil # This is the commit message #5: replacing gcc install with build-essential # This is the commit message #6: forcing yes to install anything # This is the commit message #7: ignoring blocks in pyquil notebook for tests # This is the commit message #8: linted; unnecessary import removed from notebook # This is the commit message #9: removed all references of circuits and replaced those with programs; pyquil removed from method names # This is the commit message #10: notebook rerun after changes # This is the commit message #11: pyquil program conversion removes unnecessary method

# This is the 1st commit message: starting pyquil support # This is the commit message #1: pyquil support added # This is the commit message #2: first version of pyquil notebook added # This is the commit message #3: pyquil dependency added to setup # This is the commit message #4: trying gcc install for pyquil # This is the commit message #5: replacing gcc install with build-essential # This is the commit message #6: forcing yes to install anything # This is the commit message #7: ignoring blocks in pyquil notebook for tests # This is the commit message #8: linted; unnecessary import removed from notebook # This is the commit message #9: removed all references of circuits and replaced those with programs; pyquil removed from method names # This is the commit message #10: notebook rerun after changes # This is the commit message #11: pyquil program conversion removes unnecessary method # This is the commit message #12: Public methods are bundled as __all__; changed ported over to notebooks; base object removed; repr method created # This is the commit message #13: additional error handling for new repr method # This is the commit message #14: repr method updated with class instance as input

…to pyquil-orb

qctrlopencontrols/pyquil/program.py

charmasaur · 2019-07-02T03:53:53Z

qctrlopencontrols/pyquil/program.py

+            offset_distance = 0.0
+
+        if offset_distance < 0:
+            raise ArgumentsValueError("Offsets cannot be placed properly",


Technically ArgumentsValueError is only supposed to be used when some of the arguments are invalid (and we can pinpoint exactly what's wrong with those arguments); is there a more suitable error we could use instead?

That is valid point; However, Open-Controls for now only has ArgumentsValueError

Ah, didn't realise that. Let's wait and see what Michael says, but I don't think we need to block this PR on that (since we'll need to make changes across the board).

I think ArgumentsValueError is right, the DDS is the problem here. But:

The error message is not clear enough, you should articulate specifically what the problem with the DDS is, in a manner that can help figure out how to fix it. It appears this occurs when the spacing of the gates is smaller than the gate block time? Is that right?

The dictionary returned with the ArgumentsValueError should always be a list of strings with the exact name of the argument that is a problem. So in this case it should at least be the dynamical_decoupling_sequence. The extras keyword can be used to provide a dictionary with other additional useful information.

Why are you taking a str? You should be returning the object itself in the Arguments Value Error. If it's to avoid printing the repr rather than the str, I think the repr should be printed, it's typically more informative for diagnosing errors.

You're right. Somehow I'd thought that we were generating the DDS internally in this method.

qctrlopencontrols/pyquil/program.py

charmasaur · 2019-07-02T04:02:23Z

qctrlopencontrols/pyquil/program.py

+        zero_pulses = np.isclose(rotations, 0.0).astype(np.int)
+        nonzero_pulse_counts = 3 - np.sum(zero_pulses)
+        if nonzero_pulse_counts > 1:
+            raise ArgumentsValueError(


Again, this isn't really an ArgumentsValueError is it? In this case I think it means we messed something up, because we produced an invalid DDS?

Again, Open-Controls only has ArgumentsValueError

See before.

Specifically here, the offset, rabi_rotation, azimuthal_angle and detuning rotation should all be in the extras dictionary.

qctrlopencontrols/pyquil/program.py

tests/test_pyquil_sequence.py

rajibchakravorty · 2019-07-02T06:05:43Z

Note to @michaelhush

Due to comments (valid points) here changes are made in other conversion methods to make everything consistent.
There is a comment on ArgumentsValueError being not-suitable for some errors; I agree; Do you want to consider building some more suitable Exception/Error classes?

charmasaur

Looks good. IMO we can deal with the exceptions later.

One meta-question: it looks as though a lot of the conversions are really similar (that is, we iterate over the DDS, keeping track of the current time, and insert identities or gates as necessary), and as a result there's a lot of ~duplicated code -- do you think it would be possible (and useful) to write a single shared helper method that does the work for all the conversions?

The sort of thing I have in mind is:

we create a new internal interface called CircuitBuilder or something, with methods add_identity(qubit index), add_rx(angle, qubit index), add_ry(...), add_rz(...)
we create a helper method _convert_dds_to_circuit(circuit_builder, dds, num_qubits, gate_time, algorithm), which looks very similar to the current convert_dds_to_* we have already, but uses the circuit_builder to build the circuit
for the specific convert_dds_to_pyquil/convert_dds_to_qisket/etc, we just call _convert_dds_to_circuit but provide a custom implementation of the CircuitBuilder interface. For example, for cirq it would look like:

class CirqCircuitBuilder(CircuitBuilder):
    def __init__(self, target_qubit_indices):
        self._qubits = [cirq.LineQubit(i) for i in target_qubit_indices]
        self._gate_list = []
    def add_identity(self, qubit_index):
        self._gate_list.append(cirq.I(self._qubits[qubit_index]))
    def add_rx(self, angle, qubit_index):
         self._gate_list.append(cirq.Rx(angle)(self._qubits[qubit_index]))
    ...

I could be missing something -- maybe the different libraries are more different than I realised, and in that case we definitely shouldn't try to unify things that shouldn't be unified. But if they are all basically the same, just with slightly different names/conventions, something like that could help make the code more reliable and easier to change.

Thoughts?

charmasaur · 2019-07-02T06:24:09Z

qctrlopencontrols/pyquil/program.py

+            offset_distance = 0.0
+
+        if offset_distance < 0:
+            raise ArgumentsValueError("Offsets cannot be placed properly",


Ah, didn't realise that. Let's wait and see what Michael says, but I don't think we need to block this PR on that (since we'll need to make changes across the board).

qctrlopencontrols/pyquil/program.py

charmasaur · 2019-07-02T07:41:56Z

Also, to be clear, I'm not proposing we make that CircuitBuilder change now, even if it does happen to be a good idea. Just putting it out there as a possible future project.

rajibchakravorty · 2019-07-02T08:30:46Z

Also, to be clear, I'm not proposing we make that CircuitBuilder change now, even if it does happen to be a good idea. Just putting it out there as a possible future project.

@charmasaur - I agree. Now that we have support for 3 libraries and know what is needed - it could be done the way you are suggesting. There are subtle variations (e.g. target qubits, cirq has a Schedule etc.) that may be unique to a certain library. Therefore, even if not now, we can clean up the code a whole lot more by removing duplications where possible.

Rajib Chakravorty and others added 21 commits June 12, 2019 14:18

__init__ file merged

df46e59

method in test updated

fde8a5e

adopted new standardization of packages

6901aa0

notebook updated for tests on rigetti device

fce5648

pyquil notebook with 2 segments

e79c4b1

merged with origin

f31b609

version of pyquil notebook working with Rigetti lattice

d80b066

cleaned up pyquil program

593a5c1

linted

ba024e9

merged with master

d4d4de5

Merge workflows into pyquil branch

07d5ad8

Update setup.py/README.rst to match pyproject.toml/README.md [ci skip]

3b002b8

Reapply notebook removal

bbcdebf

Merge branch 'pyquil-orb' of github.com:qctrl/python-open-controls in…

83dae62

…to pyquil-orb

Update setup.py/README.rst to match pyproject.toml/README.md [ci skip]

7242301

Further notebook removal

16db959

Add Pyquil

e45b56d

Merge branch 'pyquil-orb' into pyquil

d2faee6

Fix conflict

4efc0ba

rajibchakravorty requested review from charmasaur and michaelhush July 2, 2019 03:42

charmasaur suggested changes Jul 2, 2019

View reviewed changes

Reviews addressed

e484741

charmasaur approved these changes Jul 2, 2019

View reviewed changes

error messages updated

fb4abca

michaelhush approved these changes Jul 8, 2019

View reviewed changes

michaelhush merged commit 00fe233 into master Jul 8, 2019

michaelhush deleted the pyquil branch July 8, 2019 05:31

pyQuil support in Open-Controls #14

pyQuil support in Open-Controls #14

Uh oh!

Conversation

rajibchakravorty commented Jul 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajibchakravorty commented Jul 2, 2019

Uh oh!

charmasaur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charmasaur commented Jul 2, 2019

Uh oh!

rajibchakravorty commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants