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

Deprecate PauliTable #7245

Closed
wants to merge 11 commits into from
Closed

Conversation

ikkoham
Copy link
Contributor

@ikkoham ikkoham commented Nov 10, 2021

Summary

  • Deprecate PauliTable
  • Deprecate StabilizerTable.pauli
  • Make StabilizerTable a standalone (i.e. independent from PauliTable)

Details and comments

TODO

  • Add Releasenote

@coveralls
Copy link

coveralls commented Nov 10, 2021

Pull Request Test Coverage Report for Build 1519291394

  • 151 of 167 (90.42%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 82.868%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/quantum_info/operators/symplectic/stabilizer_table.py 144 160 90.0%
Files with Coverage Reduction New Missed Lines %
qiskit/quantum_info/operators/symplectic/pauli_table.py 1 89.35%
qiskit/quantum_info/operators/symplectic/stabilizer_table.py 2 88.4%
Totals Coverage Status
Change from base Build 1517139552: 0.01%
Covered Lines: 50271
Relevant Lines: 60664

💛 - Coveralls

@ikkoham ikkoham marked this pull request as ready for review November 10, 2021 14:25
@ikkoham ikkoham requested review from chriseclectic and a team as code owners November 10, 2021 14:25
@ikkoham
Copy link
Contributor Author

ikkoham commented Nov 10, 2021

Milestone: 0.19
Label: Changelog: Deprecation, mod: quantum info

@ikkoham ikkoham changed the title [WIP] Deprecate PauliTable Deprecate PauliTable Nov 10, 2021
@ShellyGarion ShellyGarion added this to the 0.19 milestone Nov 10, 2021
@ShellyGarion ShellyGarion added mod: quantum info Related to the Quantum Info module (States & Operators) Changelog: Deprecation Include in "Deprecated" section of changelog labels Nov 10, 2021
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.

Deprecating PauliTable is good, however keeping StabilizerTable is avoiding the problem that the API for StabilizerTable (which is the same as PauliTable) is outdated and needs to go too since it differs quite drastically in some cases from PauliList.

There is no real need for a separate StabilierTable class when its functionality is a strict subset of functionality of PauliList. The only place StabilizerTable is used is in the Clifford class where the differences are StabilizerTable uses a Z_2 phase instead of Z_4, and stores the table as a single array instead of X and Z arrays separately. This can be addressed by directly adding phase and array setter and getter methods to the Clifford class that convert the PauliList to these formats as they were in the old StabilizerTable (stack X and Z, convert phase to/from Z_2). Then all the internal code in Clifford should just has to replace obj.table.array and obj.table.phase with obj.array and obj.phase.

The stabilizer and stabilizer methods can be updated to return the internal PauliLists, and the table method itself could either return the full PauliList or be deprecated.

@jakelishman
Copy link
Member

@chriseclectic: I'd recommend against having non-trivial @property getter methods; if x.phase constructs a new array on every access, it's very very easy to introduce big performance issues without noticing. If you're going to have data transformations on the class, it's probably best to put them in a "proper" function (so don't use @property), so it's clear at the call site that it's an expensive call - calling x.phase() multiple times looks wrong, which makes it safer from a performance perspective.

@ikkoham
Copy link
Contributor Author

ikkoham commented Nov 30, 2021

I'll modify this PR after #7269. (So, don't merge this)

@ikkoham ikkoham marked this pull request as draft November 30, 2021 15:36
@jakelishman jakelishman self-assigned this Nov 30, 2021
@mtreinish mtreinish modified the milestones: 0.19, 0.20 Dec 2, 2021
@jakelishman jakelishman modified the milestones: 0.20, 0.21 Mar 30, 2022
@mtreinish mtreinish modified the milestones: 0.21, 0.22 Jun 14, 2022
@mtreinish mtreinish removed this from the 0.22 milestone Sep 23, 2022
@ikkoham
Copy link
Contributor Author

ikkoham commented Sep 28, 2022

I'll deprecate PauliTable and StabilzierTable at the same time in 0.23.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog mod: quantum info Related to the Quantum Info module (States & Operators) priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants