Skip to content

Fix string and standard gate mismatch in commutation checker (backport #13991) #14004

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

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Mar 12, 2025

Summary

This commit reworks the internals of the CommutationChecker to not rely on operation name except for where we do a lookup by name in the commutation library provided (which is the only key available to support custom gates). This fixes the case where a custom gate that overloads the standard gate name, previously the code would assume it to be a standard gate and internally panic when it wasn't. When working with standard gates (or standard instructions) we don't need to rely on string matching because we can rely on the rust data model to do the heavy lifting for us. This commit moves all the explicit handling of standard gates to use the StandardGate type directly and makes this logic more robust.

This also removes are usage of the once_cell library in qiskit-accelerate because it was used to create a lazy static hashsets of strings which are no longer needed because static lookup tables replace this when we stopped using string comparisons.

Fixes #13988

Details and comments


This is an automatic backport of pull request #13991 done by [Mergify](https://mergify.com).

* Fix string and standard gate mismatch in commutation checker

This commit reworks the internals of the CommutationChecker to not rely
on operation name except for where we do a lookup by name in the
commutation library provided (which is the only key available to support
custom gates). This fixes the case where a custom gate that overloads
the standard gate name, previously the code would assume it to be a
standard gate and internally panic when it wasn't. When working with
standard gates (or standard instructions) we don't need to rely on
string matching because we can rely on the rust data model to do the
heavy lifting for us. This commit moves all the explicit handling of
standard gates to use the StandardGate type directly and makes this
logic more robust.

This also removes are usage of the once_cell library in
qiskit-accelerate because it was used to create a lazy static hashsets
of strings which are no longer needed because static lookup tables replace
this when we stopped using string comparisons.

Fixes #13988

* Rename is_commutation_skipped() is_parameterized()

(cherry picked from commit abb0cf9)

# Conflicts:
#	crates/accelerate/src/commutation_checker.rs
@mergify mergify bot requested a review from a team as a code owner March 12, 2025 14:04
@mergify mergify bot added the conflicts used by mergify when there are conflicts in a port label Mar 12, 2025
Copy link
Contributor Author

mergify bot commented Mar 12, 2025

Cherry-pick of abb0cf9 has failed:

On branch mergify/bp/stable/2.0/pr-13991
Your branch is up to date with 'origin/stable/2.0'.

You are currently cherry-picking commit abb0cf9db.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   Cargo.lock
	modified:   crates/accelerate/Cargo.toml
	modified:   test/python/transpiler/test_commutative_cancellation.py

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   crates/accelerate/src/commutation_checker.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@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 following people are relevant to this code:

  • @Qiskit/terra-core

@github-actions github-actions bot added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Mar 12, 2025
@github-actions github-actions bot added this to the 2.0.0 milestone Mar 12, 2025
@ElePT ElePT removed the conflicts used by mergify when there are conflicts in a port label Mar 12, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13813704376

Details

  • 47 of 91 (51.65%) changed or added relevant lines in 1 file are covered.
  • 21 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.03%) to 88.099%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/commutation_checker.rs 47 91 51.65%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 94.69%
crates/circuit/src/operations.rs 3 91.27%
crates/qasm2/src/lex.rs 5 92.73%
crates/qasm2/src/parse.rs 12 96.68%
Totals Coverage Status
Change from base Build 13794763546: -0.03%
Covered Lines: 72625
Relevant Lines: 82436

💛 - Coveralls

@mtreinish mtreinish added this pull request to the merge queue Mar 12, 2025
Merged via the queue into stable/2.0 with commit 3cb7e81 Mar 12, 2025
22 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants