transpile ansatz and pauli-lists separately in VQE #6454

hhorii · 2021-05-24T08:45:38Z

Summary

A parameterized circuit of ansatz is transpiled once in VQE

Details and comments

A parameterized circuit of ansatz is copied to multiple circuits in OpFlow and they are transpiled though they share the same ansatz.
This PR transpiles ansatz and puli-lists separately and generates multiple circuits by combining them.

transpile ansatz
extract qubit-mapping from transpiled ansatz
transpile pauli-lists with extracted qubit-mapping
merge transpled ansatz and pauli-lists

This issue will resolve a part of #6432.

Cryoris · 2021-05-25T09:18:49Z

Couldn't we just solve this issue by transpiling the circuit before calling expectation.convert in the VQE (and then maybe skipping the transpilation in the circuit sampler)?

hhorii · 2021-05-25T12:46:56Z

Couldn't we just solve this issue by transpiling the circuit before calling expectation.convert in the VQE (and then maybe skipping the transpilation in the circuit sampler)?

f4aec15 does so. But it is not correct if coupling_map is specified and the final mapping is different in transpile.

Cryoris · 2021-05-25T13:47:17Z

What if we pre-transpile but then still transpile again in the circuit sampler? Since we already did most the optimizations that should be pretty fast, no?

I'm not sure the steps you outlined above are the right approach. This scenario of ansatz + Pauli expectations is very common and we should improve the transpilation in general, not only within the VQE. What about making PauliExpectation.convert return a special type of ListOp where the CircuitSampler knows how to efficiently transpile it?

woodsp-ibm · 2021-05-25T14:40:16Z

I guess going forwards this can be related to #6451
In the short term, the transpile is a one time thing right for all the circuits. For each eval they are then set with parameters and executed. So this is really speeding up their creation. And of course with include_custom things work faster as there is just the one ansatz circuit for the expectation.

hhorii · 2021-05-27T07:36:32Z

What if we pre-transpile but then still transpile again in the circuit sampler? Since we already did most the optimizations that should be pretty fast, no?

In Macbook Pro, original UCCSD circuits are traspiled in 620 seconds. Pre-transpiled circuits are transpiled in 129 seconds. Pre-transpile takes 4 second.

Cryoris · 2021-05-28T06:25:50Z

Thanks for checking the runtimes @hhorii! I agree this is quite a strong speedup and we should definitely allow to only transpile once. To keep the old behavior available, should we add a flag that allows to keep the old behavior (e.g. if you run on real HW you might want to transpile twice)?

…n VQE

woodsp-ibm · 2021-06-11T17:01:42Z

qiskit/opflow/converters/circuit_sampler.py


+    # pylint: disable=unused-argument
+    @deprecate_arguments({"circuit_sfns": "operator"})
    def sample_circuits(


I am wondering about the extent of the changes here. Should we be keeping sample_circuits for people to use as it was and add a new method that takes operator to do what we want now. If I had done code that used sample_circuits - its a public interface here after all - I think the changes here means it no longer works for my existing code - deprecated means it should keep on working as it had done until we remove the deprecated function.

It is true. However, I personally feel uncomfortable that this method, which has no unit tests, is public. Considering the future maintenance, I think the method should be deprecated now. I can't think of a use case for this method by itself.

@woodsp-ibm Thanks a lot. I finally decided not to change this method.

Cryoris

Hi @ikkoham! I have a few questions about this PR 🙂

I'm sure I fully understand the approach, would you mind explaining it briefly? How do you know the common circuit is the right-hand side state in the composed op? What if you have multiple composed ops?
Couldn't we just transpile the circuit in VQE and not transpile it again in the circuit sampler? Then we wouldn't have to change a lot (or maybe nothing at all) in the Circuit Sampler.

Cryoris · 2021-06-15T15:18:51Z

qiskit/algorithms/minimum_eigen_solvers/vqe.py

@@ -318,7 +324,7 @@ def construct_expectation(

        observable_meas = self.expectation.convert(StateFn(operator, is_measurement=True))
        ansatz_circuit_op = CircuitStateFn(wave_function)
-        return observable_meas.compose(ansatz_circuit_op).reduce()


Why is the call to reduce removed? 🙂

This change removes copies ansatz's quantum circuit. In order to take advantage of lazy evaluation, we should reduce at a later stage. Since CircuitSampler do the reduce, we do not need to do the reduce at this stage, and it will degrade the performance.

Cryoris · 2021-06-15T15:34:23Z

qiskit/transpiler/layout.py

-            del self._p2v[key]
            del self._v2p[self._p2v[key]]
+            del self._p2v[key]
        elif isinstance(key, Qubit):
-            del self._v2p[key]
            del self._p2v[self._v2p[key]]
+            del self._v2p[key]


This file should probably not be touched

Why not? This is clearly a bug, @hhorii found. This is necessary to execute our code. Should we separate the PR?

ikkoham · 2021-06-16T00:34:46Z

@Cryoris Thanks. This approach is to compile ansatz circuit only once, instead of N times when N Pauli are given.

How do you know the common circuit is the right-hand side state in the composed op? What if you have multiple composed ops?

In terms of OpFlow's design, ansatz is contained in StateFn on the right side, and information about observables is contained in MeasurementFn (=~StateFn) on the left side. Also, it is not possible to create or evaluate a ComposedOp with more than three StateFn. We don't need optimize the performance except for the case which this PR supports for now.

Couldn't we just transpile the circuit in VQE and not transpile it again in the circuit sampler? Then we wouldn't have to change a lot (or maybe nothing at all) in the Circuit Sampler.

It is my understanding that the CircuitSampler should be responsible for transpiling quantum circuits, not the VQE.
It might be a good idea to create a class that is responsible for transpiling, such as CircuitTranspiler as a Opflow Converter.

…kit-terra into transpile_only_ansatz

ikkoham · 2021-06-24T06:56:08Z

@Qiskit/terra-core Is this PR mergeable?

I've made both, and personally it's better to have the transpile closed in CircuitSampler. But this is a performance issue that needs to be included in the next release (0.18.0). So if committers really wants to transpile on the VQE side, I will complete #6602 asap.

woodsp-ibm · 2021-06-24T14:40:33Z

@Cryoris you had comments against this approach to start with - can you respond to the above please.

itoko · 2021-06-29T09:10:40Z

qiskit/opflow/converters/circuit_sampler.py

+                        diff_circuits.append(diff_circuit)
+
+                    if transpiled_common_circuit._layout is not None:
+                        layout = transpiled_common_circuit._layout.copy()


I suppose we need to set the "final" layout here, however, _layout stores the initial layout of the transpiled common circuit.

Unfortunately, we have no simple way to get the final layout of a transpiled circuit so far.
The following could work (I know it's not smart. Does anyone think of any better idea?):

Adding dummy measures to the common circuit before transpiling

Constructing the final layout from virtuals-clbits and physical-clbits maps stored in original and transpiled measures respectively

Removing the dummy measures from the common circuit

Thank you. You are right. We assume _layout is the final layout.
I have fixed the PR from your advice. Is this correct? 7418ac2

I have shown how one can get the required final layout (permutation) here: #6827

Thank you so much. It's really useful.

ajavadia · 2021-06-29T16:08:56Z

I think this solution is nice because it is common and not specific to VQE, but it points to a deficiency in the software architecture. This code has to reverse-engineer a circuit to figure out what the ansatz is, transpile that, and then glue it back.
I think we should refactor the code a bit to make this nicer. In particular, I think the common task we want to perform here is to calculate expectation values of a circuit with respect to some operator(s). We should have a canonical way of doing this in Qiskit, which we can then optimize to be fast at transpiling etc. The algorithms (VQE, QAOA, etc.) should be refactored to use this expval interface.
I think @Cryoris has some ideas on how to implement this, so let's hold off on merging this until the new expval architecture emerges. Related to #6451

ikkoham · 2021-06-29T23:52:21Z

@ajavadia Thank you so much.
This assumption (what is ansatz, what is measurement) is natural in the opflow, for example:
https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/opflow/expectations/pauli_expectation.py#L97-L98
So I thought this is enough.

However, I agree we need more refactoring here. CircuitSampler (especially cache structure) is too complicated. I think many caches should be in the operator itself (e.g. reduce()), and we should split translation and sampling like CircuitTranspiler (#6454 (comment)). I thought this performance improvement is priority and I had to include it in this release, so I tried to force it in with minimal changes, but it's obviously better to refactor it in the long run. At that time, I hope that we will be able to easily take the final layout.

Then there's the unnecessary calculation of taking sqrt and squaring it in the current method. I think this unnecessary calculation should be removed. Also, we need an API that can take variance or standard error. With the existing CircuitSampler, I cannot create a method to use the SaveExpectationValueVariance to calculate variance #5743. I have tried to fix this by experiment class but I cannot do it in the current architecture.

Finally in my opinion, I thought it would be a better idea to make a major revision when the expectation value experiment class come in the future.

ajavadia · 2021-06-30T01:27:25Z

@ikkoham thanks these are all great points. Let's gather these requirements under #6451 and then design the interfaces.

What do you mean by the "expectation value experiment class"?

The issue with final_layout: is it related to #5350 and #5280?

woodsp-ibm · 2022-06-17T20:28:30Z

@hhorii I think this can be closed right, given primitives will deal with aspects like this going forwards and VQE will be refactored to run using primitives.

ikkoham · 2022-06-23T01:38:51Z

Yes, this is the logic already applied on the qiskit-ibm-runtime side (precisely, server side), so we can close it. Thank you for pointing out.

transpile only ansatz with default

f4aec15

hhorii requested review from manoelmarques, woodsp-ibm and a team as code owners May 24, 2021 08:45

woodsp-ibm added mod: algorithms Related to the Algorithms module mod: opflow Related to the Opflow module labels May 24, 2021

mtreinish added this to the 0.18 milestone May 25, 2021

hhorii and others added 4 commits June 4, 2021 18:35

Merge remote-tracking branch 'upstream/main' into transpile_only_ansatz

b065320

split-transpile with considration of layout in vqe

e02a061

remove common_circuit as params

6106e83

correct type information in CircuitSampler

f00d82d

hhorii changed the title ~~[WIP] transpile ansatz and pauli-lists separately in VQE~~ transpile ansatz and pauli-lists separately in VQE Jun 10, 2021

ikkoham and others added 2 commits June 10, 2021 22:09

refactor and lint

8948a8f

remove unused qubits from layout when split-transpilation is enable i…

bab18b5

…n VQE

hhorii force-pushed the transpile_only_ansatz branch from 0338cbc to bab18b5 Compare June 11, 2021 07:21

woodsp-ibm reviewed Jun 11, 2021

View reviewed changes

Cryoris reviewed Jun 15, 2021

View reviewed changes

ikkoham added 6 commits June 16, 2021 14:37

move transpile from sample_circuits to convert

ce72422

Merge branch 'main' into transpile_only_ansatz

a698c92

Merge branch 'main' into transpile_only_ansatz

ee73ba9

Merge branch 'main' into transpile_only_ansatz

d3b80d1

Merge branch 'main' into transpile_only_ansatz

eba2bd4

Merge branch 'transpile_only_ansatz' of https://github.com/hhorii/qis…

16cf929

…kit-terra into transpile_only_ansatz

ikkoham added 4 commits June 17, 2021 19:42

fix lint

7b13a91

remove unnecessary params

c65902a

remove param stateless vqe doesn't use

d25c5c7

Merge branch 'main' into transpile_only_ansatz

853d64d

ikkoham mentioned this pull request Jun 18, 2021

transpile ansatz and pauli-lists separately in VQE #6602

Closed

1 task

ikkoham added 3 commits June 21, 2021 19:15

Merge branch 'main' into transpile_only_ansatz

3a160b3

Merge branch 'main' into transpile_only_ansatz

535e0ee

Merge branch 'main' into transpile_only_ansatz

951fd54

ikkoham added 2 commits June 28, 2021 14:31

Merge branch 'main' into transpile_only_ansatz

5e002a0

Merge branch 'main' into transpile_only_ansatz

30b34c6

itoko reviewed Jun 29, 2021

View reviewed changes

ikkoham added 3 commits June 29, 2021 20:10

final layout hack

7418ac2

Merge branch 'main' into transpile_only_ansatz

63dc784

fix black

410538e

ajavadia added the performance label Jun 29, 2021

add releasenote

6990e36

kdk removed this from the 0.18 milestone Jun 29, 2021

ajavadia added the on hold Can not fix yet label Jun 29, 2021

fix the bug

08b51c8

ikkoham closed this Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transpile ansatz and pauli-lists separately in VQE #6454

transpile ansatz and pauli-lists separately in VQE #6454

hhorii commented May 24, 2021 •

edited

Loading

Cryoris commented May 25, 2021 •

edited

Loading

hhorii commented May 25, 2021

Cryoris commented May 25, 2021

woodsp-ibm commented May 25, 2021

hhorii commented May 27, 2021

Cryoris commented May 28, 2021

woodsp-ibm Jun 11, 2021

ikkoham Jun 15, 2021

ikkoham Jun 18, 2021

Cryoris left a comment

Cryoris Jun 15, 2021

ikkoham Jun 16, 2021 •

edited

Loading

Cryoris Jun 15, 2021

ikkoham Jun 16, 2021 •

edited

Loading

ikkoham commented Jun 16, 2021 •

edited

Loading

ikkoham commented Jun 24, 2021

woodsp-ibm commented Jun 24, 2021

itoko Jun 29, 2021

ikkoham Jun 29, 2021

nonhermitian Jul 28, 2021

ikkoham Aug 2, 2021

ajavadia commented Jun 29, 2021 •

edited

Loading

ikkoham commented Jun 29, 2021

ajavadia commented Jun 30, 2021

woodsp-ibm commented Jun 17, 2022

ikkoham commented Jun 23, 2022

transpile ansatz and pauli-lists separately in VQE #6454

transpile ansatz and pauli-lists separately in VQE #6454

Conversation

hhorii commented May 24, 2021 • edited Loading

Summary

Details and comments

Cryoris commented May 25, 2021 • edited Loading

hhorii commented May 25, 2021

Cryoris commented May 25, 2021

woodsp-ibm commented May 25, 2021

hhorii commented May 27, 2021

Cryoris commented May 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cryoris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikkoham Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikkoham Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

ikkoham commented Jun 16, 2021 • edited Loading

ikkoham commented Jun 24, 2021

woodsp-ibm commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajavadia commented Jun 29, 2021 • edited Loading

ikkoham commented Jun 29, 2021

ajavadia commented Jun 30, 2021

woodsp-ibm commented Jun 17, 2022

ikkoham commented Jun 23, 2022

hhorii commented May 24, 2021 •

edited

Loading

Cryoris commented May 25, 2021 •

edited

Loading

ikkoham Jun 16, 2021 •

edited

Loading

ikkoham Jun 16, 2021 •

edited

Loading

ikkoham commented Jun 16, 2021 •

edited

Loading

ajavadia commented Jun 29, 2021 •

edited

Loading