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.

Sign up for GitHub

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

Jump to bottom

Refactor z2symmetries #9227

Merged
merged 19 commits into from
Dec 21, 2022
Merged

Refactor z2symmetries #9227

merged 19 commits into from
Dec 21, 2022

Conversation

Anthony-Gandon
Copy link
Contributor

Summary

Fix #9140
This PR adds an implementation of the Z2symmetries class based on SparsePauliOp.
The tests of the main Z2Symmetry class were adapted and considered as based requirements of this class.
Overall the changes are minor because most of the logic is shared between PauliSumOp and SparsePauliOp.

@Anthony-Gandon Anthony-Gandon requested review from a team and ikkoham as code owners December 2, 2022 04:15
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Dec 2, 2022
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@mrossinek mrossinek self-requested a review December 2, 2022 07:12
@coveralls
Copy link

coveralls commented Dec 2, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3745399651

  • 134 of 166 (80.72%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 84.572%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/quantum_info/analysis/z2_symmetries.py 134 166 80.72%
Files with Coverage Reduction New Missed Lines %
src/sabre_swap/layer.rs 2 98.95%
Totals Coverage Status
Change from base Build 3743830724: 0.02%
Covered Lines: 64031
Relevant Lines: 75712
💛 - Coveralls

mrossinek
mrossinek reviewed Dec 2, 2022
View reviewed changes
Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

Alright this is a rather lengthy review but a lot of these comments revolve around documentation. Other than that I just suggest some minor refactorings and code changes.

Overall this looks like its in good shape 👍

@mrossinek mrossinek removed the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Dec 2, 2022
@Anthony-Gandon
Copy link
Contributor Author

So following one of your comments @mrossinek, I'm proposing a refactored version of the find_z2_symmetries() method (with the three Z_or_I, X_or_I, Y_or_I cases). I'm not sure the test bench covers all cases so to be sure I have a personal jupyter for testing : gist.
I also tried to clarify the logic behind

mrossinek
mrossinek reviewed Dec 5, 2022
View reviewed changes
Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

Just a quick second round of reviews (I ran out of time).

I compared the logic which you implemented here with the original one and as far as I could see everything seems to be correct 👍 Thanks a lot, this implementation looks a lot better!

ikkoham
ikkoham reviewed Dec 5, 2022
View reviewed changes
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

Thank you. Nice PR. I left some comments.

mrossinek
mrossinek reviewed Dec 13, 2022
View reviewed changes
Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

Some more suggestions but other than that, this looks good to me!

Thanks a lot, Anthony! 🙂

@mrossinek mrossinek requested a review from ikkoham December 13, 2022 12:32
mrossinek
mrossinek previously approved these changes Dec 16, 2022
View reviewed changes
Copy link
Member

@mrossinek mrossinek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks a lot Anthony! 👍

EDIT: you just need to run make black once to fix that formatting error

ikkoham
ikkoham previously approved these changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@ikkoham ikkoham left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for late review. LGTM! Thank you for using PauliList!
(I will try to improve Clifford part as my review comments above)

@ikkoham ikkoham added mod: quantum info Related to the Quantum Info module (States & Operators) Changelog: New Feature Include in the "Added" section of the changelog labels Dec 21, 2022
mrossinek
mrossinek approved these changes Dec 21, 2022
View reviewed changes
Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

LGTM, too! Thanks a lot, Anthony! 👍

@mergify mergify bot merged commit 049451d into Qiskit:main Dec 21, 2022
Cryoris pushed a commit to Cryoris/qiskit-terra that referenced this pull request Jan 12, 2023
@Anthony-Gandon @ikkoham @Cryoris
Refactor z2symmetries (Qiskit#9227)
26e23ae 
* first step

* first step and new files

* pass all tests

* Add release note and fix mypy

* First batch of corrections

* Second batch of Fixes: subtests, docstring, qiskiterror

* Fix release note

* First working attempt to factor find_Z2_symmetries

* Moved _sparse_pauli_op_is_zero to Z2symmetries

* rename find_Z2_symmetries to find_z2_symmetries

* Fix docstrings and refactor _test_symmetry_at_row_col method

* rm logging and fix type hints

* add return types

* fix lint

Co-authored-by: ikkoham <ikkoham@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman left review comments

@woodsp-ibm woodsp-ibm woodsp-ibm left review comments

@ikkoham ikkoham ikkoham approved these changes

@mrossinek mrossinek mrossinek approved these changes

Assignees
No one assigned
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: quantum info Related to the Quantum Info module (States & Operators)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Z2Symmetries
7 participants
@Anthony-Gandon @qiskit-bot @coveralls @jakelishman @ikkoham @mrossinek @woodsp-ibm