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 PauliList class #5993

Merged
merged 15 commits into from
Jun 29, 2021
Merged

Add PauliList class #5993

merged 15 commits into from
Jun 29, 2021

Conversation

chriseclectic
Copy link
Member

@chriseclectic chriseclectic commented Mar 9, 2021

Summary

Add PauliList to replace PauliTable and StabilizerTable

Details and comments

This class functions as an optimized list of the new Pauli class (including phase).

  • tests
  • Improve documentation

@chriseclectic chriseclectic requested a review from a team as a code owner March 9, 2021 13:59
@chriseclectic chriseclectic marked this pull request as draft March 9, 2021 13:59
@chriseclectic chriseclectic marked this pull request as ready for review May 20, 2021 16:59
@chriseclectic chriseclectic changed the title [WIP] Add PauliList class Add PauliList class May 20, 2021
"Index {} is greater than number of qubits"
" in the PauliList ({})".format(ind, self.num_qubits)
)
# TODO: Fix phase
Copy link
Member

Choose a reason for hiding this comment

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

is this TODO still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your pointing out. I forget fixing the phase.


# Convert quantum circuits to Cliffords
if isinstance(other, Clifford):
other = other.to_circuit()
Copy link
Member

@ShellyGarion ShellyGarion Jun 7, 2021

Choose a reason for hiding this comment

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

can you maybe do it directly without decomposing the clifford? like its done in #6297

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I've moved the _evolve_clifford to the base class.

value = PauliList(target)
value[0] = "II"
self.assertEqual(value, target)

Copy link
Member

Choose a reason for hiding this comment

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

In the init tests - maybe worth checking more phases with i and -i

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I added the tests.

target = PauliList(labels1 + labels2)
self.assertEqual(target, pauli1 + pauli2)

def test_add_qargs(self):
Copy link
Member

@ShellyGarion ShellyGarion Jun 7, 2021

Choose a reason for hiding this comment

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

can test also pseudo-random paulis

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Why do we need the test for the random Paulis here?

target = PauliList(["IIII", "YYYY", "YIXI", "ZIYI"])
self.assertEqual(pauli1 + pauli2([3, 1]), target)

def test_getitem_methods(self):
Copy link
Member

Choose a reason for hiding this comment

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

maybe test more phases, and also pseudo-random paulis


def test_from_labels_1q(self):
"""Test 1-qubit from_labels method."""
labels = ["I", "Z", "Z", "X", "Y"]
Copy link
Member

Choose a reason for hiding this comment

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

Z appears twice

Copy link
Contributor

Choose a reason for hiding this comment

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

This is as intended. We also see that the same Pauli is not omitted.

np.array([[False], [True], [True], [False], [True]]),
np.array([[False], [False], [False], [True], [True]]),
)
target = ["I", "Z", "Z", "X", "Y"]
Copy link
Member

Choose a reason for hiding this comment

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

Z appears twice

value = pauli.to_labels(array=True)
self.assertTrue(np.all(value == target))

def test_labels_round_trip(self):
Copy link
Member

Choose a reason for hiding this comment

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

can try more phases as well as pseudo-random paulis



@ddt
class TestPauliListOperator(QiskitTestCase):
Copy link
Member

@ShellyGarion ShellyGarion Jun 7, 2021

Choose a reason for hiding this comment

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

in operators tests can test more phases



@ddt
class TestPauliListMethods(QiskitTestCase):
Copy link
Member

@ShellyGarion ShellyGarion Jun 7, 2021

Choose a reason for hiding this comment

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

in sorting tests can test with phases

value = PauliList(labels).unique()
self.assertEqual(target, value)

def test_delete(self):
Copy link
Member

Choose a reason for hiding this comment

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

in delete and insert tests - maybe add phases?

value = pauli.insert(1, PauliList(i), qubit=True)
self.assertEqual(value, target1)

def test_commutes(self):
Copy link
Member

Choose a reason for hiding this comment

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

in commute / anti-commute tests - maybe add phases?

ShellyGarion
ShellyGarion previously approved these changes Jun 24, 2021
t-imamichi
t-imamichi previously approved these changes Jun 24, 2021
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM

@mtreinish mtreinish added automerge Changelog: New Feature Include in the "Added" section of the changelog and removed automerge labels Jun 25, 2021
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Actually I missed it initially, this needs a release note documenting the new class. After that's added this is ready to go

@ikkoham
Copy link
Contributor

ikkoham commented Jun 28, 2021

@mtreinish Thank you. I made a PR to add the release note. chriseclectic#25

@chriseclectic Could you merge this PR if it's OK?

@mtreinish mtreinish dismissed stale reviews from t-imamichi and ShellyGarion via 2733e2d June 28, 2021 10:08
mtreinish
mtreinish previously approved these changes Jun 28, 2021
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, I just merged @ikkoham's commit with the release notes from chriseclectic#25

@mtreinish
Copy link
Member

@ikkoham
Copy link
Contributor

ikkoham commented Jun 28, 2021

OK. I'll fix this soon.

@ikkoham
Copy link
Contributor

ikkoham commented Jun 28, 2021

Thank you. I think chriseclectic#26 fixes the error on CI. (I've tested on my local and it passed.)

@mergify mergify bot merged commit 89c4f6e into Qiskit:main Jun 29, 2021
garrison added a commit to garrison/qiskit that referenced this pull request Dec 22, 2021
I believe this is what was intended when the example was originally added
(Qiskit#5993), as this now matches the method signature, and the `phase`
on the previous line is no longer an unused variable.
mergify bot pushed a commit that referenced this pull request Dec 23, 2021
I believe this is what was intended when the example was originally added
(#5993), as this now matches the method signature, and the `phase`
on the previous line is no longer an unused variable.
mergify bot pushed a commit that referenced this pull request Dec 23, 2021
I believe this is what was intended when the example was originally added
(#5993), as this now matches the method signature, and the `phase`
on the previous line is no longer an unused variable.

(cherry picked from commit 8e54baa)
mergify bot added a commit that referenced this pull request Dec 23, 2021
I believe this is what was intended when the example was originally added
(#5993), as this now matches the method signature, and the `phase`
on the previous line is no longer an unused variable.

(cherry picked from commit 8e54baa)

Co-authored-by: Jim Garrison <[email protected]>
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.

6 participants