Skip to content

fixed issue 8438 PauliList.evolve raises exception#8453

Closed
BramDo wants to merge 8 commits into
Qiskit:mainfrom
BramDo:fixissue8438branchv2
Closed

fixed issue 8438 PauliList.evolve raises exception#8453
BramDo wants to merge 8 commits into
Qiskit:mainfrom
BramDo:fixissue8438branchv2

Conversation

@BramDo
Copy link
Copy Markdown

@BramDo BramDo commented Aug 4, 2022

Summary

This pull request is fix for issue #8438
Title
PauliList.evolve raises exception when evolving certain Clifford circuits

Details and comments

changed in file base_pauli.py

if dtype is None:
dtype = np.min_scalar_type(x.shape[axis])

@BramDo BramDo requested review from a team and ikkoham as code owners August 4, 2022 15:22
@qiskit-bot
Copy link
Copy Markdown
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:

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Aug 4, 2022

CLA assistant check
All committers have signed the CLA.

@ikkoham
Copy link
Copy Markdown
Contributor

ikkoham commented Aug 5, 2022

Great! Thank you for fixing the bug. This fix looks good.
Could you add some tests for this problem and release note?

@ikkoham ikkoham added Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. mod: quantum info Related to the Quantum Info module (States & Operators) stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. labels Aug 5, 2022
@ikkoham ikkoham added this to the 0.21.2 milestone Aug 5, 2022
@coveralls
Copy link
Copy Markdown

coveralls commented Aug 5, 2022

Pull Request Test Coverage Report for Build 2818990759

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 84.054%

Totals Coverage Status
Change from base Build 2800102724: 0.02%
Covered Lines: 56100
Relevant Lines: 66743

💛 - Coveralls

@BramDo
Copy link
Copy Markdown
Author

BramDo commented Aug 5, 2022

Great! Thank you for fixing the bug. This fix looks good. Could you add some tests for this problem and release note?

I will add the test and release not, but I have to find out how to do this.

@BramDo
Copy link
Copy Markdown
Author

BramDo commented Aug 7, 2022

added test
test_evolve_clifford_with_phase()

added release note
[releasenotes/notes/fix_evolve_raises_exception_in_Cliffordcircuits

Copy link
Copy Markdown
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. LGTM, I left a minor comment.

Comment thread test/python/quantum_info/operators/symplectic/test_pauli.py Outdated
Comment thread test/python/quantum_info/operators/symplectic/test_pauli.py Outdated
BramDo and others added 2 commits August 8, 2022 17:17
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Copy link
Copy Markdown
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. LGTM

Copy link
Copy Markdown
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I'm not certain this is the fully correct solution to the problem here - see #8438 (comment) for my other reasoning.

@HuangJunye HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 16, 2022
@ikkoham
Copy link
Copy Markdown
Contributor

ikkoham commented Aug 17, 2022

@jakelishman You are completely right. We should pass dtype when we call _count_y if it takes mod 2 or 4. This fix would be a performance regression, so it was a not good. (I withdraw my initial approval.)

@mtreinish mtreinish removed the stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. label Aug 19, 2022
@mtreinish mtreinish removed this from the 0.21.2 milestone Aug 19, 2022
@BramDo
Copy link
Copy Markdown
Author

BramDo commented Aug 23, 2022

As already discussed it is not necessary to modify the _count_y because dtype handling there looks correct for what the function does. The more correct solution is to ensure that whatever's calling that is passing np.uint8 as the dtype when it's going to bind the result to a uint8 context, or it manually cast. The caller can be found in the exception handling of the example:

~/anaconda3/envs/qiskit/lib/python3.9/site-packages/qiskit/quantum_info/operators/symplectic/base_pauli.py in _evolve_cz(base_pauli, q1, q2
661 base_pauli._z[:, q1] ^= x2
662 base_pauli._z[:, q2] ^= x1
--> 663 base_pauli._phase += 2 * np.logical_and(x1, x2).T
664 return base_pauli
665

In the example we use an evolve over the phase. This is done by the evolve_cz function. In line 663 the phase vector is set by:
base_pauli._phase += 2 * np.logical_and(x1, x2).T

To cast the phase vector to a uint8 we can do:
base_pauli._phase += np.uint8(2 * (np.logical_and(x1, x2).T))

Summary
No change in the _count_y function but modification of the result of the _evolve_cz function. This works, I got the mathematically correct answer as before and we don't have to change the phase vector from what it logically should be (u8) to something much wider (i64) with the problem of using to much data all the time.

I'll be happy to go further with this PR if this is the way to go.

@BramDo BramDo requested a review from jakelishman August 23, 2022 12:00
@jakelishman
Copy link
Copy Markdown
Member

Sorry for the lack of responses here. The underlying issue appears to have been fixed by #8877, so I'll close this PR as obsolete now, but please feel free to open another PR if I've missed anything.

@BramDo BramDo deleted the fixissue8438branchv2 branch January 29, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: quantum info Related to the Quantum Info module (States & Operators)

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants