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

Remove transpiler optimizations from the benchmarks #282

Merged
merged 3 commits into from
Jul 19, 2019

Conversation

atilag
Copy link
Member

@atilag atilag commented Jul 11, 2019

Summary

Transpilation process is not mandatory in simulation, and it can add different optimizations to the circuits over time that will interfere in the results of the benchmarks, so we are removing it from all the benchmarks.

Anyway, I want to make sure that this assumption is correct @chriseclectic :)

Details and comments

We may notice visible performance peak in the benchmarks right after this PR, that is expected.

@atilag atilag requested a review from chriseclectic July 11, 2019 16:33
@chriseclectic
Copy link
Member

@atilag I disagree. Transpiling to u3, cx was intentional to force the arbitrary 2-qubit untiaries to be decomposed into CX and single-qubit gates, which make different implementations of quantum volume circuits equivalent. Otherwise you might have 2-qubit arbitrary unitaries that are simulated directly (if the circuit was generated that way) so you are not benchmarking core performance of single and two qubit gates, or optimizations like gate fusion.

The transpilation time shouldn't be included in the benchmark though, only the simulation time of the qobj after assembly.

If we are worried about other transpilation passes changing performance due to changing optimizations we should just make a custom passmanager for benchmarks that only uses the unroller and no other transpilation passes.

@atilag
Copy link
Member Author

atilag commented Jul 12, 2019

Ok, so let's keep transpilation but disable optimizations, the unrolling will still apply.
@kdk suggested to set a fixed value for seed_transpiler too.

@atilag atilag changed the title Remove transpilation step from the benchmarks Remove transpiler optimizations from the benchmarks Jul 12, 2019
  the seed.
* All benchmarks unroll to u1,u2,u2 and cx basis gates
@atilag atilag force-pushed the remove-transpile-benchmarks branch from cc53c8e to 1bf1240 Compare July 12, 2019 10:12
@chriseclectic
Copy link
Member

@atilag After thinking more on this I think what we really want two three tiers of benchmarks:

  • Benchmarks for low-level functions (applying matrices, kraus, etc)
  • Benchmarks for synthetic applications (Ideal ignis, aqua algorithms, etc)
  • benchmarks for realistic applications (noisy ignis, aqua algorithms, etc)

All of these are necessary for assessing PRs for performance gains, however the second point could also be used for comparing Aer to other simulators (which we don't do yet but should), and the third is the most useful for "real world" uses of the simulator by researchers. In that last two cases we would want to report whatever gives to fastest overall execution time (likely doesn't involve unrolling).

Quantum volume is a bit odd as it could fit into all of those cases, and at the moment I was thinking of it more as the former case (benchmarking random u3 and cx gates), or the latter (circuit is designed to benchmark device quality in the presence of errors). In both those cases you would have to runroll to simulate the noise model at gate level.

However if we want to consider QV as a synthetic application, then the best overall execution time, which would be to build the circuit using random SU(4) unitary matrices (not cx,u3) and then then unrolling as you have done in this PR: This is because the unroller decomposition of SU(4) into CX,u3 gates is not optimal. One issue here is several different pieces of code used to generate quantum volume circuits and not all are equal (some use cx, u3 directly, others use SU(4) operators). Once quantum volume is in ignis we will have a "canonical" circuit that can be used for evaluation.

@atilag
Copy link
Member Author

atilag commented Jul 15, 2019

This makes a lot of sense.
I would merge this PR, which is an improvement, and address the changes you mention in a follow up.

@chriseclectic chriseclectic merged commit 558b0bb into Qiskit:master Jul 19, 2019
hhorii pushed a commit to hhorii/qiskit-aer that referenced this pull request Jul 25, 2019
@atilag atilag deleted the remove-transpile-benchmarks branch August 9, 2019 09:42
dcmckayibm pushed a commit to dcmckayibm/qiskit-aer that referenced this pull request Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants