Skip to content

Conversation

@rajibchakravorty
Copy link
Contributor

Major : Add method to convert a DDS to a cirq.Circuit or cirq.Schedule

Request for review:

  1. circuit_type is an option with 2 possible string values - a) 'standard circuit' - where the circuit will contain sequence operations and gaps will be filled up by identity gates; returns a cirq.Circuit instance b) 'scheduled circuit' - where sequence operations are scheduled to be occurring at offsets and does not require any Identity gates to fill; returns a cirq.Schedule instance; in either case the returned instance is accepted by cirq.Simulator for simulation (both are illustrated in the notebook. Please check if the string keys make sense.

  2. Example notebook in examples/run_a_dds_on_cirq.ipynb.

Other additions/points to consider:

  1. Pre-Post gates are specified here as a . 2x2 unitary matrix. This can accommodate the default X_{pi/2} or other rotations too (similar to angle notation in U3 gate in Qiskit)
  2. The drawing of circuit is not great in cirq. Especially the 2x2 matrix at both ends looks bad (see the example notebook). But I could not find a nicer way to draw the circuit. Same comment applies to result - no nice direct API to draw/beatify the outcome.
  3. In the absence of an actual device, demonstration of noisy system was not possible [so removed the comparison with Ramsey sequence]. Alternative was to use artificial noise models. But the library provides limited noisy gates/channels and the knowledge of noise had to known a-priori. I would consider this as beyond scope of the notebook at this moment.

with a string as key. The string is formatted as 'qubit-X' where
X is a number between 0 and len(target_qubits).
circuit_type : str, optional
One of 'scheduled circuit' or 'standard circuit'. In the case of
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 really sure about python best practice here, but instead you could refer to constants.SCHEDULED_CIRCUIT and constants.STANDARD_CIRCUIT. That might make it a bit harder to pass an invalid value (even though we don't have a compiler, people might use IDEs that warn them about using non-existent constants).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with legacy here. If this needs to be changes, we have to check carefully other similar instances.

@michaelhush
Copy link
Contributor

Comments:

  1. There is some inconsistency with the naming. Currently we use IBM Q and cirq in the same way which isn't quite right. I think we should change the example file, and accordingly title, for running_a_dds_on_ibm_q.ipynb to export_a_dds_to_qiskit.ipynb. This actually matches the description of the file in the readme, so that helps. This also means we should change cirq example to export_a_dds_to_cirq.ipynb.

  2. Add a brief description of the Export a DDS to Cirq jupytet notebook to the README.

@rajibchakravorty
Copy link
Contributor Author

  • Notebook names and titles changed
  • Add a couple of sentences on Cirq notebook in Readme.

@michaelhush
Copy link
Contributor

Consistency comments:

With qiskit, we have a qiskit module then we have the method convert_dds_to_quantum_circuit which returns a QuantumCircuit object. The file is called quantum_circuit.py. I want to match this pattern as much as possible.

Cirq currently has a method convert_dds_to_cirq_circuit which returns two different types of objects, Circuits or Schedules. It also has the term cirq inserted. This breaks the pattern above.

To change the cirq module to match the qiskit module we should remove this "circuit_type" keyword altogether (and it's constants) and instead use two method:

convert_dds_to_circuit
convert_dds_to_schedule

Each method now returns a predictable type, which is clearer. They are implied to be for cirq through the module. Technically they should also be in separate files circuit.py and schedule.py to keep the pattern consistent. If there is any shared code I wouldn't mind it being in one of them and imported by the other. But just make sure there is not mutual importing errors.

@rajibchakravorty
Copy link
Contributor Author

updates:

  1. Conversion to cirq circuit and schedule separated in two files (with common methods imported from qiskit module where needed
  2. Notebooks updated accordingly. Minor change in qiskit notebook also (e.g. matching the gate_time in description and actual code and some tex format for micorseconds etc.).

from qctrlopencontrols.dynamic_decoupling_sequences import DynamicDecouplingSequence
from qctrlopencontrols.exceptions import ArgumentsValueError

from qctrlopencontrols.qiskit import (
Copy link
Contributor

Choose a reason for hiding this comment

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

cirq should not import anything from qiskit. Could you pull these constants into global and put the method in a module where it would be more appropriate.

@rajibchakravorty
Copy link
Contributor Author

@michaelhush

  • Troubling methods are removed - so no cross-module imports necessary anymore

@michaelhush michaelhush merged commit 58a8ceb into master May 30, 2019
@michaelhush michaelhush deleted the cirq branch June 4, 2019 05:44
rajibchakravorty pushed a commit that referenced this pull request Jun 12, 2019
# 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
rajibchakravorty pushed a commit that referenced this pull request Jun 12, 2019
# 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants