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

Folding functions should retain circuit properties (e.g., num shots) and device information (e.g., qubits, connectivity, etc.) #9

Closed
rmlarose opened this issue Feb 14, 2020 · 9 comments

Comments

@rmlarose
Copy link
Contributor

Our current folding techniques for Cirq build the folded circuit from an empty circuit. Circuits in Cirq can be tied to a device which tells information about qubits, connectivity, and gate operation times. As such, I think it's more appropriate to copy the input circuit and insert gates to do the folding. Thoughts?

@andreamari
Copy link
Member

I think it's a good point. Moreover the same problem probably exists also for PyQuil circuits (e.g. num_shots can be a property of a circuit).

I agree with your solution, otherwise a simple alternative is also this one:
empty_circuit = input_circuit[0:0]
which creates an empty circuit with the same properties of the input and should work for both Cirq and PyQuil circuits.

@rmlarose
Copy link
Contributor Author

Good point about pyQuil circuits and properties.

I tested

empty_circuit = input_circuit[0:0]

and it didn't copy over the shots.

I'll update the name of this issue to reflect the importance for pyQuil as well.

@rmlarose rmlarose changed the title Cirq folding functions should retain device information Folding functions should retain circuit properties (e.g., num shots) and device information (e.g., qubits, connectivity, etc.) Feb 15, 2020
@andreamari
Copy link
Member

andreamari commented Feb 18, 2020

It is true that empty_circuit = input_circuit[0:0] doesn't copy num_shots. But I also noticed that circuit_sum= input_circuit + input_circuit doesn't copy num_shots to circuit_sum !
So I wonder if this is a bug or a feature.

An empty circuit in PyQuil can be correctly created in this way
empty_circuit = input_circuit.copy_everything_except_instructions()

however, even in this case, num_shots would be lost as soon as we start making simple operations (e.g., additions).

@andreamari
Copy link
Member

andreamari commented Feb 19, 2020

I will make a PR (#11 ) to improve the creation of empty circuits in PyQuil using copy_everything_except_instructions(). I am also adding some tests associated to this issue.

The problem with num_shots and similar properties remains. But I am quite sure that this is a bug of PyQuil. See, e.g., the __add__ and __getitem__ method of the Program class where it is evident that properties are not retained (https://github.com/rigetti/pyquil/blob/master/pyquil/quil.py).

@willzeng
Copy link
Contributor

@andreamari does the issue appear specific to num_shots instructions? If so, perhaps dedicated code should exist to pop off this instruction?

Alternatively, we could code for with the assumption that the user does not choose numshots for things to be mitigated. When you run a given Program through mitigation it isn't clear what the numshots should do as the Mitigator will have discretion to choose optimal amounts of shots to take etc

@willzeng
Copy link
Contributor

Of the two approaches I just suggested. I think I prefer stripping num_shots from an input program and emitting a warning to the user that it is being ignored.

@andreamari
Copy link
Member

It is not just self.num_shots, there are also other properties like: self._defined_gates and self.native_quil_metadata, which are lost when using the + operator or slicing a circuit [ ].

I think I got what is the solution: one should only append instructions (or circuit slices) with the inst method or equivalently with +=. In this way all the properties of the input circuit are preserved. In other words: a = a + b is bad, while a += b should correctly preserve the properties of a.

@willzeng
Copy link
Contributor

ah gotcha. This is an unexpected idiosyncrasy indeed. Does that mean that replacing all a=a+b with a+=b will fix the issue? If so, it is likely time for a pull request :)

@andreamari
Copy link
Member

This issue is now solved by #11 .

willzeng added a commit that referenced this issue Apr 17, 2020
# This is the 1st commit message:

simplify cirq getting started and add qiskit getting started

# This is the commit message #2:

add tests of qiskit getting started

# This is the commit message #3:

update qiskit example with conversions

# This is the commit message #4:

move seeds

# This is the commit message #5:

add doctests to main getting started

# This is the commit message #6:

better seed

# This is the commit message #7:

set seeds

# This is the commit message #8:

update

# This is the commit message #9:

qiskit name

# This is the commit message #10:

line

# This is the commit message #11:

update doc tests and remove redundant tests

# This is the commit message #12:

fix apidoc
rmlarose pushed a commit that referenced this issue Apr 22, 2020
* # This is a combination of 12 commits.
# This is the 1st commit message:

simplify cirq getting started and add qiskit getting started

# This is the commit message #2:

add tests of qiskit getting started

# This is the commit message #3:

update qiskit example with conversions

# This is the commit message #4:

move seeds

# This is the commit message #5:

add doctests to main getting started

# This is the commit message #6:

better seed

# This is the commit message #7:

set seeds

# This is the commit message #8:

update

# This is the commit message #9:

qiskit name

# This is the commit message #10:

line

# This is the commit message #11:

update doc tests and remove redundant tests

# This is the commit message #12:

fix apidoc

* update getting started

* reformat testcode as block

* switch to make doctest

* include change to make doctest in workflow file

* add doctest to workflow

* test documentation in ci with doctest

* remove lines to prompt build

* revert to original ci file

* add new name run with pytest-sphinx

* add sphinx to install

* add cd

* add sphinx dependencies and extentions to ci

* purposefully introduce error to check ci build fails

* revert to passing build

* add documentation testing to readme

* remove README extra file

* update zne.py for new keep type default

* move random seed to test

* make test factories seed local

* make noise mitigation more clear

Co-authored-by: Nathan Shammah <nathan.shammah@gmail.com>
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

No branches or pull requests

3 participants