Add approximation_degree to CommutativeCancellation and wire presets - #16679
Add approximation_degree to CommutativeCancellation and wire presets#16679philipjpark wants to merge 8 commits into
Conversation
Pass pass_manager_config.approximation_degree to CommutativeCancellation at all three preset call sites, add test_approximation_degree, and add a release note. Completes the integration work for Qiskit#14115. Co-authored-by: Cursor <cursoragent@cursor.com>
The reproduction script was useful for Phase II validation but should not be included in the upstream pull request for Qiskit#14115. Co-authored-by: Cursor <cursoragent@cursor.com>
Removed the Phase II reproduction script so the branch only contains the approximation_degree pass wiring, preset changes, test, and release note.
Local validation venv was committed in the Week 6 cleanup commit; drop it and ignore .venv-test so the upstream PR only contains the Qiskit#14115 fix files. Co-authored-by: Cursor <cursoragent@cursor.com>
…, test, and release note
Keep the Qiskit PR limited to the Qiskit#14115 fix files only. Co-authored-by: Cursor <cursoragent@cursor.com>
qiskit-bot
added
the
Community PR
|
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:
|
|
|
|
@gadial Initially started reviewing it but I found that there are some pending PRs already for this same issue. |
|
Thanks @jeevan0920, yes, I noted the other open PRs (#14802, #15777, #16491) on #14115 as well. Happy to wait on the maintainer's guidance and make any updates to this PR if needed. |
alexanderivrii
left a comment
alexanderivrii
left a comment
There was a problem hiding this comment.
Overall this looks fine, I have left some comments inline.
| approximation_degree: The threshold used in the average gate fidelity | ||
| computation to decide whether pairs of gates can be considered as | ||
| canceling or commuting. This can be a floating point value between | ||
| 0 and 1, or ``None``. If the value is ``None`` the default fidelity | ||
| of ``1.0`` is used. |
There was a problem hiding this comment.
Note that the pass in rust only handles floating-point values for approximation_degree, and not None. Based on the discussion and the decision when integrating SubstitutePi4Rotations and CommutativeOptimization into the transpiler pipeline, for now it's best to restrict the Python pass to also only work with floating-points values.
| approximation_degree = ( | ||
| self._approximation_degree | ||
| if self._approximation_degree is not None | ||
| else 1.0 | ||
| ) |
There was a problem hiding this comment.
Based on the previous comment, it's best to add this "if approximation degree is None, then set it to 1." to builtin_plugins.py.
| for approximation_degree in [None, 0.5, 1.0]: | ||
| with self.subTest(approximation_degree=approximation_degree): | ||
| result = CommutativeCancellation(approximation_degree=approximation_degree)(qc) | ||
| self.assertEqual(expected, result) |
There was a problem hiding this comment.
Arguably this test does not check that approximation_degree affects the pass in any way. Can you think of a circuit where CommutativeCancellation would behave differently when approximation_degree is set to 1.0 (no approximation) vs. say 0.0 (maximal approximation)?
Also, per previous comments, we do not want to support None just yet. But maybe worth checking that not setting approximation_degree gives the same behavior as setting it to 1.0.
5 participants
Why was this PR needed?
Fixes #14115.
transpile(..., approximation_degree=...)already reaches passes likeRemoveIdentityEquivalent, but notCommutativeCancellation: the Python pass never accepted or forwarded the degree to Rustcancel_commutations(which already supports it), and the three preset call sites never passedpass_manager_config.approximation_degree. Cancellation always ran at full precision (1.0).What does this PR do?
Adds
approximation_degree: float | None = 1.0toCommutativeCancellation, forwards it tocancel_commutations(None→1.0), wires it at all three preset sites, and addstest_approximation_degreeplus a release note.What are the relevant issue numbers?
Closes #14115
Screenshots / Recordings (if applicable)
Before:
CommutativeCancellationhad noapproximation_degree; Rust binding already did.After: Cancellation OK for
None/0.5/1.0— ALL TESTS PASSEDDoes this PR meet the acceptance criteria?
AI/LLM disclosure