Use ParameterReferences type for ParameterTable values

[kevinhartman]

Summary

Changes ParameterTable value type from List[(Instruction, int)] to a new collection type, ParameterReferences, which is set-like, and no longer seq-like.

ParameterReferences:

A set of instruction parameter slot references. Items are expected in the form (instruction, param_index). Membership testing is overriden such that items that are otherwise value-wise equal are still considered distinct if their instructions are referentially distinct.

Details and comments

It's not actually necessary to update existing uses of ParameterTable or its values, but usage is updated in QuantumCircuit for performance (mostly to avoid creating lists that are then immediately spooled into ParameterReferences). See ParameterReferences docstring for more information.

Fixes #6725. ParameterReferences is set-like, so there's no need to perform a linear search in _update_parameter_table to check existing references for every instruction parameter.

[kevinhartman]

Tagging @jakelishman esp. for Python best practice insights :)

[coveralls]

Pull Request Test Coverage Report for Build 2468737188

• 55 of 60 (91.67%) changed or added relevant lines in 6 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.0007%) to 84.426%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/parametertable.py	38	43	88.37%

Totals
Change from base Build 2465465477:	-0.0007%
Covered Lines:	54710
Relevant Lines:	64802

---

💛 - Coveralls

[jakelishman]

This looks pretty sensible, and looks like it should reduce a potential quadratic complexity in circuit depth when using parameters. Do you have benchmarks for improvement for it to hand?

[kevinhartman]

Do you have benchmarks for improvement for it to hand?

I don't! I should have some time to get some numbers in the coming week, and will report back.

[kevinhartman]

@jakelishman, I found at least one place that depends on ParameterReferences being seq-like. Here, for each reference, we need to look up the corresponding copy of the same reference in a new circuit(s).

https://github.com/Qiskit/qiskit-terra/blob/5ff5757a762a677baa7db2fde09559ee87a0c8dc/qiskit/opflow/gradients/circuit_gradients/param_shift.py#L208-L218

I'm not really sure if there's a good way to do this if ParameterReferences is no longer ordered, since ParameterReferences might (by design) contain multiple references that are equivalent, but not the same. We can't do any sort of lookup since we're dealing with a deep copy in the new circuit.

Any ideas on how we should proceed?

[kevinhartman]

Here are the asv performance results for time_build_parameterized_circuit, which show a significant speedup for large numbers of gates that re-use a comparatively small set of parameters.

  84.6±1ms         74.6±1ms     0.88  circuit_construction.ParameterizedCircuitConstructionBench.time_build_parameterized_circuit(20, 2048, 8)
1.47±0.03s       1.20±0.01s     0.82  circuit_construction.ParameterizedCircuitConstructionBench.time_build_parameterized_circuit(20, 32768, 128)
   484±6ms          307±3ms     0.63  circuit_construction.ParameterizedCircuitConstructionBench.time_build_parameterized_circuit(20, 8192, 8)
14.8±0.06s       4.91±0.03s     0.33  circuit_construction.ParameterizedCircuitConstructionBench.time_build_parameterized_circuit(20, 131072, 128)
4.23±0.04s       1.23±0.01s     0.29  circuit_construction.ParameterizedCircuitConstructionBench.time_build_parameterized_circuit(20, 32768, 8)
   2.15±0m       5.02±0.03s     0.04  circuit_construction.ParameterizedCircuitConstructionBench.time_build_parameterized_circuit(20, 131072, 8)

[ewinston]

Is there an asv test for a large number of gates with a large number of parameters?

[jakelishman]

@ewinston: Yeah, the same benchmark suite Kevin showed goes up to 131,072 gates x 131,072 parameters, but I'm guessing he just didn't show them here because there wasn't much change (and in total there's ~36 benchmarks in that suite).

[jakelishman]

@kevinhartman: sorry, I haven't had chance to look properly, but I feel like that code block there is logically just trying to match up the instructions between circuits, including from a main circuit. I think it might be possible to modify the algorithm in that location a little such that it iterates over a index into the QuantumCircuit.data list, and then uses that index for each circuit to retrieve the instruction tuple, from which it can access the parameter table. I'm not 100% sure, but I'd be hopeful we can change things on the algorithm side rather than requiring ParameterReferences to be sequence-like, because in the end, we don't really want circuit data to have a total ordering - the instruction flow is logically a graph, not a sequence.

(I get that my proposed solution uses the total ordering of data, but I'm mostly just pushing the issue down the line, because when we're ready to move the backing data structure of QuantumCircuit.data to a graph-like one, we'll have to solve these problems far more generally across Terra.)

[kevinhartman]

I think it might be possible to modify the algorithm in that location a little such that it iterates over a index into the QuantumCircuit.data list, and then uses that index for each circuit to retrieve the instruction tuple, from which it can access the parameter table.

Hmm, alright! That's helpful. I'll give it a try.

(I get that my proposed solution uses the total ordering of data, but I'm mostly just pushing the issue down the line, because when we're ready to move the backing data structure of QuantumCircuit.data to a graph-like one, we'll have to solve these problems far more generally across Terra.)

I agree completely that it'd be nice to eliminate maintaining an arbitrary ordering of parameter references, at the very least.

[jakelishman]

Overall this looks solid, and the benchmarks you've got above look absolutely 🔥. I have a point worrying about pickle (using id as a key is basically encoding dangling-pointer risks as a feature, so we need to be very careful), and I think some of the tests still assume the ordered behaviour from a previous iteration of this PR.

Don't forget the release note - we like to trumpet our speed-ups!

[jakelishman]

Looks good to me, and the speed-ups are great! Thanks!