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

Feature: Approximate 1Q synthesis #7084

Closed
wants to merge 19 commits into from
Closed

Feature: Approximate 1Q synthesis #7084

wants to merge 19 commits into from

Conversation

ecpeterson
Copy link
Contributor

@ecpeterson ecpeterson commented Sep 30, 2021

cc @levbishop , @1ucian0

Summary

This (draft) PR synthesizes 1Q operators up to approximation. Aside from inherent utility, I hope that this stems the warnings reported in #7033 and #7042. This carries on (though in a different direction) from Lev's comment.

Details and comments

The changed regions of code are:

  1. _circuit_psx_gen takes (optional) fidelity estimates for the circuit generated by xfun and xpifun and selects a circuit from a few templates based on best approximation + fidelity distance from the target + operational cost.
  2. The _circuit_* wrappers take an (optional) fidelity_mapping argument which reports fidelity estimates for operations appearing on the relevant qubit. At the moment, this is a Callable so that I can bind the qubit index in the backend property lookup, but passing around anonymous lambdas is not totally desirable. At the moment, this takes the form of a defaultdict, typically populated from whatever BackendProperties is around.
  3. Optimize1qGatesDecomposition receives a BackendProperties object and threads that information through to the decomposition calls.
  4. When checking whether the returned synthesis is worse than the synthesis already prepared, it compares the two fidelity costs rather than gate count (except in degenerate cases, e.g., when no error info is available).

Some missing features are:

  • I haven't installed the initial couple of pipes (e.g., in the default pass manager constructors) which provide the backend property objects to the 1Q synthesis passes. Without these, the 'default' synthesis route proceeds functionally as before, without considering the per-gate cost of the steps in a 1Q circuit, with some vague preference for shorter strings when they're available.
  • It's mildly (but not completely) undertested.

I would particularly appreciate comments on:

  • The data flow which pipes the relevant error metrics down to the synthesis routine feels clumsy to me.
  • I avoided installing the generic 'approximation' knob present in the cousin 2Q synthesis routines, which may or may not have been a good idea. As written, this PR explicitly assumes that the error rates reported are average gate fidelities, which is a stronger assumption than promised by the backend, and without which I'm not sure this really suffices to mute the warning.

Comment on lines +173 to +181
# incorporate a possible trace distance from approximate synthesis
operator = old_run[0].op.to_matrix()
for gate in old_run[1:]:
operator = gate.op.to_matrix().dot(operator)
decomp_unitary = new_circ[0][0].to_matrix() if new_circ else np.eye(2)
for gate, _, _ in new_circ[1:]:
decomp_unitary = gate.to_matrix().dot(decomp_unitary)
trace_pairing = np.trace(decomp_unitary @ np.conj(operator.data).transpose(1, 0))
new_infidelity += (4 - abs(trace_pairing) ** 2) / 6
Copy link
Member

Choose a reason for hiding this comment

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

Have you done any perf testing? If this recalculation of the infidelity becomes a bottleneck we may prefer to pass it out from the decomposer along with the decomposed circuit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, none. I expect this to have worse performance, but I couldn't guess at a factor. Matthew has previously shown me how to run benchmarks; I'll try to remember how that goes.

qiskit/quantum_info/synthesis/one_qubit_decompose.py Outdated Show resolved Hide resolved
qiskit/quantum_info/synthesis/one_qubit_decompose.py Outdated Show resolved Hide resolved
@ecpeterson
Copy link
Contributor Author

ecpeterson commented Nov 4, 2021

As written, this comes with significant performance degradation. I haven't run asv, but here's stestr run on main:

Ran: 11510 tests in 288.6423 sec.
Sum of execute time for each test: 1840.2396 sec.

versus on this branch:

Ran: 11411 tests in 575.9220 sec.
Sum of execute time for each test: 3376.0384 sec.

Not great.

@levbishop
Copy link
Member

As written, this comes with significant performance degradation. I haven't run asv, but here's stestr run on main:

Ouch.

@ecpeterson
Copy link
Contributor Author

ecpeterson commented Nov 4, 2021

As of recent commits, this is now

Ran: 11612 tests in 369.2880 sec.
Sum of execute time for each test: 2074.5102 sec.

which is still noticeably worse, but approaching tolerable.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1423654641

  • 114 of 125 (91.2%) changed or added relevant lines in 3 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.003%) to 82.378%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/quantum_info/synthesis/one_qubit_decompose.py 50 51 98.04%
qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py 61 71 85.92%
Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/uc.py 2 88.65%
qiskit/pulse/library/waveform.py 3 90.38%
Totals Coverage Status
Change from base Build 1421607931: -0.003%
Covered Lines: 49721
Relevant Lines: 60357

💛 - Coveralls

Comment on lines +195 to +197
operator = old_run[0].op.to_matrix()
for gate in old_run[1:]:
operator = gate.op.to_matrix().dot(operator)
Copy link
Member

Choose a reason for hiding this comment

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

Don't we already have this unitary previously calculated? Not sure how expensive this recalculation is, might matter for long runs

@levbishop levbishop mentioned this pull request Dec 3, 2021
@HuangJunye HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 21, 2022
@javabster
Copy link
Contributor

hi @ecpeterson are you still working on this?

@ecpeterson
Copy link
Contributor Author

@javabster My employer's attention is elsewhere, so I am no longer working on this. :) The main outstanding thread of work was some performance degradation, but who knows how the code has drifted in the intervening 18mo.

@javabster
Copy link
Contributor

Thanks for letting us know @ecpeterson, I'll close this PR but if you'd like to return to it at a later date just let us know and we can reopen

@javabster javabster closed this Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants