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

Better tolerances for gate decompositions #5827

Merged
merged 50 commits into from
Apr 14, 2021
Merged

Conversation

levbishop
Copy link
Member

@levbishop levbishop commented Feb 9, 2021

  • Fixes Should set rtol in np.isclose calls in OneQubitEulerDecomposer #5766 by setting rtol=0 in one_qubit_decompose so that simplifications happen with the expected tolerance
  • Follow-on to Global phase in TwoQubitBasisDecomposer #5764 making tests for TwoQubitBasisDecomposer test the global phase more effectively.
  • Adds an override param to TwoQubitBasisDecomposer.__call__() to force a given number of basis uses, to make testing easier
  • Adds explicit tests for decompositions using 0-,1-,2-,3-applications of basis.
  • Fixes an old copy/paste bug in test_two_qubit_weyl_decomposition_abc()
  • Adds another branch to PSX and ZSX decompositions so that simplification never happens if simplify=False
  • Adds further simplification to oneq decompositions so they can simplify to zero gates if identity-equivalent
  • Adds smoke tests for all special cases of the oneq decompositions (more thorough tests would be good)
  • Refactor test code
  • Revamp testing to be more thorough and check both with simplify=True and simplify=False
  • Look more closely at the tolerances, and due to the above fixes, now we can achieve 1.E-14 with simplify=True and 1.E-12 with simplify=False (rather than the very loose 1.E-7 we had before). Please don't loosen these tolerances again to paper over real bugs...
  • Make TwoQubitWeylDecomposition is now a factory that knows how to specialize itself up to a fidelity for special pathological cases where the decomposition would otherwise be unstable (the 2-qubit analog of Euler angle gimbal lock at the north pole of the Bloch sphere that leads us to having the simplify option on the OneQubitBasisDecomposer
  • Due to the above self-specializations behavior of TwoQubitWeylDecomposition remove the ad-hoc roundings from Round M2 dot product in Weyl decomposition #4835 Revert "Pin numpy version < 1.19.0 to unblock CI (#4599)" #4656 Restore approx 0 rounding to decomp0 method #4862 as hopefully the non-deterministic, non-repeatability is killed completely by the specializations
  • I independently noticed that np.isclose() was becoming a bottleneck as @mtreinish has proposed fixing in Tune performance of OneQubitEulerDecomposer #5915 so this gives some of the speedup from that PR. The other ideas for performance (using _append and working directly on the DAGCircuit are also worth doing, and now merged into this PR

mtreinish and others added 8 commits March 5, 2021 14:30
This commit adds a new basis ZSXX to the 1q euler decomposer. This basis
is identical to the ZSX basis except it does an additional
simplification pass so that instead of having 2 sx gates in the
synthesized output a single X gate is used instead.

Fixes Qiskit#5722
@levbishop
Copy link
Member Author

levbishop commented Mar 9, 2021

This is ready to review

  • Please take a close look at the __new__ class factory stuff. I think its fine but would like a second set of eyes on that
  • @mtreinish please check I carried over your perf improvements and run your test to check if there is any regression
  • Note I removed the TwoQubitBasisDecomposer.__call__() array conversion and unitary checking completely, rather than adding an internal _decompose method, under the philosophy that TwoQubitBasisDecomposer is an internal function anyway so can skip such validation

Possible direction for the future:
Add gate definitions for the special cases that aren't already defined (fsim, partial swap,etc) and then can add template circuits for their specific commutation rules. Not sure if the template matcher is smart enough with such templates to find always single-qubit-optimal forms after the kak decompositions have taken care to get the two-qubit part optimal (automatically generalizing https://arxiv.org/pdf/2008.08571.pdf fig 4(b))

@levbishop levbishop marked this pull request as ready for review March 9, 2021 08:14
@levbishop levbishop requested review from chriseclectic and a team as code owners March 9, 2021 08:14
@levbishop levbishop changed the title [WIP] Better tolerances for gate decompositions Better tolerances for gate decompositions Mar 9, 2021
@levbishop levbishop requested a review from mtreinish March 9, 2021 08:15
@jaygambetta
Copy link
Member

@levbishop this pr is really needed it fixes the global phase I notice for being outside 2pi much more. What is the status of getting this merged.

@levbishop
Copy link
Member Author

@levbishop this pr is really needed it fixes the global phase I notice for being outside 2pi much more. What is the status of getting this merged.

I'd also like to get it merged soon. Because it ended up expanding into something quite extensive when I fixed all the tests and chased all the edge cases, Ali and Matthew were not comfortable including it in 0.17. I think the current .plan is to get 0.17.1 out in a week or so and then merge this into master ready for 0.18. Personally, since it does fix a few known bugs, and almost certainly a bunch that we didn't know about, and I have more confidence that it's generally doing the right thing, I'd be happy backporting to 0.17 but I'm in the minority here.

jaygambetta
jaygambetta previously approved these changes Apr 10, 2021
Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

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

Looks good to me, I would like to revisit the hardcoded basis after this and come up with a strategy that does not lead to a mission creep of adding ever-so-slightly different basis over time.

ajavadia
ajavadia previously approved these changes Apr 12, 2021
Some tests were failing in TestSingleControlledRotationGates due to
different global phase tracking causing an extra P() gate
in some circumstances. Since the CCRZ gates are not minimal in single-qubit
rotations anyway, and should be single-qubit optimized afterwards
I fixed this by changing the test to pass for the slightly longer
result circuit.
@levbishop levbishop dismissed stale reviews from ajavadia and jaygambetta via 42c9803 April 13, 2021 16:05
@mergify mergify bot merged commit 2b7046f into Qiskit:master Apr 14, 2021
@levbishop levbishop deleted the tolerances branch May 21, 2021 18:55
@levbishop levbishop restored the tolerances branch June 20, 2021 23:35
@1ucian0 1ucian0 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jun 25, 2021
@levbishop levbishop deleted the tolerances branch August 12, 2021 22:53
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.

Should set rtol in np.isclose calls in OneQubitEulerDecomposer
7 participants