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

Fix 4-pi periodicity of controlled rotations in the CommutationChecker #13670

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Jan 15, 2025

Summary

The commutation checker currently wrongly assumes that the controlled Pauli rotations CRX/Y/Z are equivalent to the identity for multiples of $2\pi$. However, this is only true for $4\pi$. Note that CP is indeed $2\pi$ periodic.

Details and comments

The tests were modified to compare the output of the commutation checker to a numeric result instead of a hardcoded one, such that this error shouldn't arise in the future again.

@Cryoris Cryoris added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Jan 15, 2025
@Cryoris Cryoris added this to the 1.3.2 milestone Jan 15, 2025
@Cryoris Cryoris requested a review from a team as a code owner January 15, 2025 16:51
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@Cryoris Cryoris changed the title Fix $4\pi$ periodicity of controlled rotations in the CommutationChecker Fix 4-pi periodicity of controlled rotations in the CommutationChecker Jan 15, 2025
@coveralls
Copy link

coveralls commented Jan 15, 2025

Pull Request Test Coverage Report for Build 12811238577

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 29 of 29 (100.0%) changed or added relevant lines in 1 file are covered.
  • 17 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.003%) to 88.92%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 5 92.73%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 12790871540: -0.003%
Covered Lines: 79451
Relevant Lines: 89351

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM! This is the typical tricky bug, I just left a tiny suggestions for the test docstring.

test/python/circuit/test_commutation_checker.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderivrii alexanderivrii 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 for identifying and fixing the bug. I have left a few tiny comments.

crates/accelerate/src/commutation_checker.rs Outdated Show resolved Hide resolved
crates/accelerate/src/commutation_checker.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing the comments.

@ElePT ElePT enabled auto-merge January 16, 2025 15:00
@ElePT ElePT added this pull request to the merge queue Jan 16, 2025
Merged via the queue into Qiskit:main with commit dffc2df Jan 16, 2025
17 checks passed
@alexanderivrii alexanderivrii added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jan 16, 2025
mergify bot pushed a commit that referenced this pull request Jan 16, 2025
…er` (#13670)

* start

* fix 4pi periodicity of controlled pauli rots

* review comments by Elena & Sasha

(cherry picked from commit dffc2df)

# Conflicts:
#	crates/accelerate/src/commutation_checker.rs
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2025
…er` (#13670) (#13676)

* start

* fix 4pi periodicity of controlled pauli rots

* review comments by Elena & Sasha

(cherry picked from commit dffc2df)

# Conflicts:
#	crates/accelerate/src/commutation_checker.rs

Co-authored-by: Julien Gacon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants