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

Adding Pauli to the standard gate library #5153

Merged
merged 18 commits into from
Oct 19, 2020
Merged

Conversation

gadial
Copy link
Contributor

@gadial gadial commented Sep 30, 2020

Summary

A PauliGate which applies pauli operations (given as a string) to a set of qubits is added to the standard library.

Details and comments

While functionally equivalent to sequential application of pauli gates, this instruction can be performed efficiently by statevector simulator using a single pass on the statevector.

@gadial gadial requested a review from a team as a code owner September 30, 2020 11:03
"""
import numpy as np

from qiskit import QuantumCircuit, QuantumRegister
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from qiskit import QuantumCircuit, QuantumRegister
from qiskit.circuit.quantumcircuit import QuantumCircuit
from qiskit.circuit.quantumregister import QuantumRegister

This should solve the circular import errors

Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

Looks mostly good. I have a couple of requests for renaming variables to match quantum_info operators. It also looks like the input string is reversed from the label parameter of the Operator and Pauli classes.

Another thing I would add is to change the Pauli.to_instruction method to return one of these gate objects rather than a circuit containing Pauli gates:

class Pauli:
...
def to_instruction(self):
    from qiskit.circuit.libarary.standard_gates import PauliGate
    return PauliGate(str(self))


def __init__(self, pauli_string="I"):
if isinstance(pauli_string, float):
pauli_string = "I"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this raise an exception if its not a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is certainly a welcome behavior, but it clashes with many automatic tests which implicitly assume the parameters a gate can have consist of a vector of floats (and sometimes just assign random parameters based on this assumption). For now it's either that or modification of the testing framework.

Copy link
Member

Choose a reason for hiding this comment

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

@kdk Is the testing framework easy to modify for this? Seems not ideal to have to add this to the class just for the automatic tests

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this should be easy to fix. Which tests are failing? There are tests to keep some expectations on standard gates (e.g. that their gate parameters align with their definition parameters, ...) but they each already have cases handling gates that have non-standard signatures. In fact, it looks like those have already been updated to handle PauliGates.

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 added workaround in the test files; you might have a better idea how to circumvent this.

https://github.com/Qiskit/qiskit-terra/pull/5153/files#diff-d45d22ad301d9187912920617d10dfd53aa0c1a1c80fca08b1a02acf2552e65eR179

The most problematic case is what we have in test_to_matrix and test_to_matrix_op: here the params are initialized once params = [0.1 * i for i in range(1, 11)], before being applied to each gate; the only difference is the computed number of actual parameters used from this list.

Since PauliGate expects str and not float, this crashes without direct override (as I added).

qiskit/circuit/library/standard_gates/pauli.py Outdated Show resolved Hide resolved
qiskit/circuit/library/standard_gates/pauli.py Outdated Show resolved Hide resolved
qiskit/circuit/library/standard_gates/pauli.py Outdated Show resolved Hide resolved
qiskit/circuit/library/standard_gates/pauli.py Outdated Show resolved Hide resolved
qiskit/circuit/quantumcircuit.py Outdated Show resolved Hide resolved
qiskit/circuit/library/standard_gates/pauli.py Outdated Show resolved Hide resolved
Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

If we are storing the label in the params, we don't really need to store it also as an attribute, we can just get it from self.parameters[0] in other functions that use it. Alternatively we don't store parameters at all and modify the assemble method of this class to use the attribute rather than params.

ie.

def assemble(self):
    instruction = super().assemble()
    instruction.params = [self.pauli_string]
    return instruction

This later way might be preferable if we want to move towards having all gate parameters be floads (@ajavadia what do you think?)

qiskit/circuit/library/standard_gates/pauli.py Outdated Show resolved Hide resolved
@kdk
Copy link
Member

kdk commented Oct 14, 2020

If we are storing the label in the params, we don't really need to store it also as an attribute, we can just get it from self.parameters[0] in other functions that use it. Alternatively we don't store parameters at all and modify the assemble method of this class to use the attribute rather than params.

ie.

def assemble(self):
    instruction = super().assemble()
    instruction.params = [self.pauli_string]
    return instruction

This later way might be preferable if we want to move towards having all gate parameters be floads (@ajavadia what do you think?)

For me, params is preferable. It's more in line with how the other gate classes handle parameters, so more tooling is likely to work out of the box (drawing, assembly, ...) and with validate_parameters, there should be enough control to specify what parameters are valid for this gate type.

Also, looking now, this would be better for generalized_gates than standard_gates.

chriseclectic
chriseclectic previously approved these changes Oct 15, 2020
Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

LGTM, the only thing missing is a release note.

@chriseclectic chriseclectic changed the title [WIP] Adding Pauli to the standard gate library Adding Pauli to the standard gate library Oct 15, 2020
@chriseclectic chriseclectic added the Changelog: New Feature Include in the "Added" section of the changelog label Oct 15, 2020
@chriseclectic chriseclectic mentioned this pull request Oct 19, 2020
4 tasks
@kdk kdk merged commit 0341cdb into Qiskit:master Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants