Reduce redundant circuit copies and parameter bindings in CircuitSampler by n-ejima · Pull Request #9228 · Qiskit/qiskit

n-ejima · 2022-12-02T08:12:01Z

Summary

The current code has these two issues:

QuantumCircuit is copied at every parameter binding, but the old copies are not used repeatedly.
All of the parameters included in QuantumCircuit are updated even when the values of parameters are not changed.

I fixed the above issues by:

Copying each of transpiled circuits only once and reusing the single copy per circuit.
Only updating the parameters whose values are different from the previous ones.

This can be done because parameter values remain in transpiled circuits by reusing the copied circuits.

Details and comments

CLAassistant · 2022-12-02T08:12:06Z

All committers have signed the CLA.

CLAassistant · 2022-12-02T08:12:06Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

qiskit-bot · 2022-12-02T08:12:10Z

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 the following people are requested to review this:

@Cryoris
@Qiskit/terra-core
@manoelmarques
@woodsp-ibm

coveralls · 2022-12-02T10:21:04Z

Pull Request Test Coverage Report for Build 4742200433

55 of 55 (100.0%) changed or added relevant lines in 1 file are covered.
18 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.007%) to 85.925%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	95.14%
crates/qasm2/src/lex.rs	5	91.14%
crates/qasm2/src/parse.rs	12	97.11%

Totals
Change from base Build 4737986268:	-0.007%
Covered Lines:	71122
Relevant Lines:	82772

💛 - Coveralls

ikkoham

@n-ejima Thank you for your first contribution. Your proposal is great!

This PR seems like it would definitely improve the performance of VQE. Do you have any benchmarking results? The benchmark can show the impact of this PR.

For the code, we don't want to change the QuantumCircuit class if possible. The type signature change of .assign_parameters affects the linter (CI fails). Therefore, I propose to create a method inside CircuitSampler that is assigning parameters to QuantumCircuit.

jakelishman

This PR is suggesting a massive mix-up of object ownership semantics that I don't think is safe to bring near QuantumCircuit, and will be fragile even for use within opflow. I would suggest that if opflow deliberately wants to share ownership of various internal objects that it can guarantee no references to will leak out to users and each object will only be mutated once, it's going to need to do its own thing a little bit; I'm not comfortable adding these changes into QuantumCircuit itself.

On a second note: with the deprecation of opflow pending, is this something we need to pursue? How does the non-deprecated replacement for this behave?

woodsp-ibm · 2022-12-02T14:56:40Z

with the deprecation of opflow pending, is this something we need to pursue?

I don't think so, if the focus here is really around opflow and CircuitSampler. The author may not have known that, with the refactoring of algorithms over to using primitives in the last release, that the former algorithms and the way they worked, along with opflow and QuantumInstance they used, will shortly be deprecated and removed thereafter.

jakelishman · 2022-12-02T15:52:48Z

Thanks Steve - for sure, I wasn't suggesting it was your fault here @n-ejima, sorry that wasn't clear.

n-ejima · 2022-12-05T06:36:35Z

@n-ejima Thank you for your first contribution. Your proposal is great!

This PR seems like it would definitely improve the performance of VQE. Do you have any benchmarking results? The benchmark can show the impact of this PR.

This graph shows the processing time of parameter bindings when running VQE in below experimental environment. Circuit copy and parameter assignment account for a large part of parameter bindings.
This PR can reduce a large amount of processing time of parameter bindings.

n-ejima · 2022-12-22T07:31:50Z

I fixed the code not to change QuantumCircuit class. With this fixing, the 2nd issue I mentioned is not resolved. I think only reducing the number of circuit copy (1st issue) has enough effect to the performance.

Result of executing VQE on the H8 molecule (# of qubit:13, # of parameter:184,# of iteration:1861)
- total executing time of assign_parameter
  - before fixing: 888 sec, overall VQE executingtime: 2015 sec
  - reducing circuit copy: 167 sec (assign_parameter + processing to reduce circuit copy), overall VQE executing time: 1210 sec

ikkoham

Sorry for my lazy review.

The direction (reduce redundant circuit copies) seems nice. The performance improvement is significant and may be use the same idea in primitives.
I have minor comments to improve codes.

ikkoham · 2023-01-10T02:26:14Z

                self._transpiled_circ_cache = self.quantum_instance.transpile(
                    circuits, pass_manager=self.quantum_instance.unbound_pass_manager
                )
+


These codes should be outside of try.

ikkoham · 2023-01-10T02:26:44Z

+                self._reuse_global_phase = []
+                for circ in self._transpiled_circ_cache:
+                    shadow = circ.copy()
+                    shadow._increment_instances()


Do we need _increment_instances and _name_update here?

ikkoham · 2023-01-10T02:47:43Z

        if param_bindings is not None:
+            if self._reuse_circs != []:
+                # copy quantum circuit if len(param_bindings) > 1
+                append_size = len(param_bindings) - len(self._reuse_circs[0])


If the length of param_bindings is smaller on the second calls, won't this be a negative number?

ikkoham · 2023-01-10T02:52:39Z

+        # restore the original parameter table and global phase in shadow_circs
+        for i, circ in enumerate(self._reuse_circs):
+            for j, _ in enumerate(circ):
+                for param in self._reuse_parameter_tables[i][j]:


self._reuse_parameter_tables[i][j].items() is readable since self._reuse_parameter_tables[i][j][param] is frequently appeared.

ikkoham · 2023-01-10T02:57:43Z

+                    for k, instr in enumerate(self._reuse_parameter_tables[i][j][param]):
+                        instr[0].params = (
+                            self._preserved_parameter_tables[i][j][param]
+                            .__getstate__()[k][0]


Is it difficult to avoid using __getstatete__?

Cryoris · 2023-01-10T16:43:28Z

It's definitely great to reduce copies, however I'm not quite sure that this is the right approach here. If I understand correctly, the idea is to directly change the parameters inside the instruction.params and avoid going through .bind_parameters, right?

This approach is a bit volatile as e.g. parameter changes are not propagated down to sub-instructions and decomposing the circuit will lose the parameter assignment. There are two other ways we could implement this or speed up the assignment generally:

general speedup: make circuit copies fast by taking the parameter out of the instruction (see circuit deepcopy too slow: singleton gates #3800), this is something @jakelishman is already preparing
make parameters re-assignable: this is closer to the approach in this PR and would mean that we refactor the parameters to additionally store a value they are currently assigned to and make the backends handle these new types

ikkoham · 2023-01-11T07:32:12Z

@Cryoris Thanks. QuantumCircuit's deepcopy speedup should be done. This is a problem in many areas, not just this one.

I think the approach of making reassignable is nice. However, I personally do not want to change QC it's already too complicated. If someone could do it by 0.24, I agree that we wait for that change and then allow this PR to use re-assign.

n-ejima · 2023-01-12T00:52:00Z

@Cryoris

If I understand correctly, the idea is to directly change the parameters inside the instruction.params and avoid going through .bind_parameters, right?

I don't intended to update the parameters without assign_parameter(). In my approach, parameters are updated by assign_prameters(), and after executing the quantum circuit, parameters are restored to its original state like 0.5 * t[0] maintaining both QuantumCircuit and QC's ParameterTable have the same instruction.params instance.

make parameters re-assignable: this is closer to the approach in this PR and would mean that we refactor the parameters to additionally store a value they are currently assigned to and make the backends handle these new types

As @ikkoham said, if QC will be modified to have both original and assigned parameters, I want to wait for it.

ikkoham · 2023-02-02T07:04:02Z

@Cryoris Do you have a plan to implement re-assignable parameters for 0.24?

Cryoris · 2023-04-19T19:04:00Z

We discussed this in the team and came to the conclusion that we would rather not include this PR into the release, for two main reasons

This is a workaround for the expensive circuit copies, which we are currently working to resolve. With light-weight circuits, this workaround would not be necessary anymore and the problem is properly fixed, without the trouble of mixed ownerships mentioned above
Opflow is being deprecated in this release, so adding a workaround solution as this adds a lot of overhead to code that will be removed soon.

Your benchmarks are definitely impressive and also serve as a good indicator of how much faster Qiskit can be, once the circuits are efficiently copied 🙂 I hope this makes sense to you, let me know if you want to discuss further!

jakelishman · 2023-12-19T13:07:40Z

I'm sorry that we couldn't move this to merge at the time. Now that opflow has been removed, as of #11111, this PR is obsolete, so I'm going to close it. Thanks for the effort at the time, and sorry that it didn't merge while opflow was live.

n-ejima added 2 commits December 2, 2022 14:57

Reduce redundant circuit copies

c1a2cc2

Reduce redundant parameter bindings

98cbd1f

n-ejima requested review from a team, ikkoham, manoelmarques and woodsp-ibm as code owners December 2, 2022 08:12

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Dec 2, 2022

ikkoham suggested changes Dec 2, 2022

View reviewed changes

ikkoham added mod: opflow performance labels Dec 2, 2022

jakelishman requested changes Dec 2, 2022

View reviewed changes

Add args to assign_parameter

8584a0e

n-ejima and others added 2 commits December 22, 2022 11:12

Reduce redundant circuit copies without modifying QuantumCircuit

99f9a76

Merge branch 'main' into fix_param_update

153a4d4

n-ejima requested review from ikkoham and jakelishman and removed request for ikkoham and jakelishman December 22, 2022 07:32

ikkoham reviewed Jan 10, 2023

View reviewed changes

ikkoham added this to the 0.23.0 milestone Jan 10, 2023

ikkoham self-assigned this Jan 10, 2023

ikkoham modified the milestones: 0.23.0, 0.24.0 Jan 17, 2023

kdk changed the title ~~Reduce redundant circuit copies and parameter bindings~~ Reduce redundant circuit copies and parameter bindings in CircuitSampler Apr 6, 2023

mtreinish removed this from the 0.24.0 milestone Apr 11, 2023

ikkoham added the priority: low label Apr 14, 2023

ikkoham added this to the 0.24.0 milestone Apr 14, 2023

Merge branch 'Qiskit:main' into fix_param_update

03a174f

Cryoris removed this from the 0.24.0 milestone Apr 19, 2023

jakelishman closed this Dec 19, 2023

Conversation

n-ejima commented Dec 2, 2022

Summary

Details and comments

Uh oh!

CLAassistant commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Dec 2, 2022

Uh oh!

qiskit-bot commented Dec 2, 2022

Uh oh!

coveralls commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4742200433

💛 - Coveralls

Uh oh!

ikkoham left a comment

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

woodsp-ibm commented Dec 2, 2022

Uh oh!

jakelishman commented Dec 2, 2022

Uh oh!

n-ejima commented Dec 5, 2022

Uh oh!

n-ejima commented Dec 22, 2022

Uh oh!

ikkoham left a comment

Choose a reason for hiding this comment

Uh oh!

ikkoham Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

ikkoham Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

ikkoham Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

ikkoham Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

ikkoham Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

Cryoris commented Jan 10, 2023

Uh oh!

ikkoham commented Jan 11, 2023

Uh oh!

n-ejima commented Jan 12, 2023

Uh oh!

ikkoham commented Feb 2, 2023

Uh oh!

Cryoris commented Apr 19, 2023

Uh oh!

jakelishman commented Dec 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

CLAassistant commented Dec 2, 2022 •

edited

Loading

coveralls commented Dec 2, 2022 •

edited

Loading