From 9769785c11385485ef2186d7aafa6d149f4d5fa3 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Fri, 22 Nov 2024 13:05:17 -0500 Subject: [PATCH 1/4] Use _append_standard_gate directly --- qiskit/circuit/library/n_local/n_local.py | 25 +++++++++---------- ...l-flatten-parameters-d7eb1858e0eaa160.yaml | 18 +++++++++++++ test/python/circuit/library/test_nlocal.py | 15 +++++++++++ 3 files changed, 45 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml diff --git a/qiskit/circuit/library/n_local/n_local.py b/qiskit/circuit/library/n_local/n_local.py index 8c0b4d285086..080d84723257 100644 --- a/qiskit/circuit/library/n_local/n_local.py +++ b/qiskit/circuit/library/n_local/n_local.py @@ -1109,11 +1109,11 @@ def _build_rotation_layer(self, circuit, param_iter, i): if k not in skipped_blocks ) for qubits in all_qubits: - instr = CircuitInstruction( - simple_block.gate(*itertools.islice(param_iter, simple_block.num_params)), + circuit._append_standard_gate( + simple_block.gate, qubits, + [next(param_iter) for _ in range(simple_block.num_params)], ) - circuit._append(instr) else: block_indices = [ list(range(k * block.num_qubits, (k + 1) * block.num_qubits)) @@ -1139,11 +1139,12 @@ def _build_entanglement_layer(self, circuit, param_iter, i): # It's actually nontrivially faster to use a listcomp and pass that to `tuple` # than to pass a generator expression directly. # pylint: disable=consider-using-generator - instr = CircuitInstruction( - simple_block.gate(*itertools.islice(param_iter, simple_block.num_params)), - tuple([target_qubits[i] for i in indices]), + qubits = tuple([target_qubits[i] for i in indices]) + circuit._append_standard_gate( + simple_block.gate, + qubits, + [next(param_iter) for _ in range(simple_block.num_params)], ) - circuit._append(instr) else: # apply the operations in the layer for indices in entangler_map: @@ -1277,7 +1278,6 @@ def get_entangler_map( _StdlibGateResult = collections.namedtuple("_StdlibGateResult", ("gate", "num_params")) -_STANDARD_GATE_MAPPING = get_standard_gate_name_mapping() def _stdlib_gate_from_simple_block(block: QuantumCircuit) -> _StdlibGateResult | None: @@ -1291,14 +1291,13 @@ def _stdlib_gate_from_simple_block(block: QuantumCircuit) -> _StdlibGateResult | if ( instruction.clbits or tuple(instruction.qubits) != tuple(block.qubits) - or ( - getattr(_STANDARD_GATE_MAPPING.get(instruction.operation.name), "base_class", None) - is not instruction.operation.base_class - ) + or getattr(instruction.operation, "_standard_gate", None) is None or tuple(instruction.operation.params) != tuple(block.parameters) ): return None - return _StdlibGateResult(instruction.operation.base_class, len(instruction.operation.params)) + return _StdlibGateResult( + instruction.operation._standard_gate, len(instruction.operation.params) + ) def _normalize_entanglement( diff --git a/releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml b/releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml new file mode 100644 index 000000000000..d7fe0e037950 --- /dev/null +++ b/releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml @@ -0,0 +1,18 @@ +--- +fixes: + - | + Fixed a bug in :class:`.NLocal` and derivatives, occurring when ``flatten=True`` was set + and standard gates backed by Rust were used. When binding parameters in-place to such a + circuit, the new parameter values were not correctly propagated to the circuit instructions. + + The circuit could still be compiled and used as expected, however, inspecting the individual + operations would still show the old parameter values. For example:: + + from qiskit.circuit.library import EfficientSU2 + + circuit = EfficientSU2(2, flatten=True) + circuit.assign_parameters([1.25] * circuit.num_parameters, inplace=True) + + print(circuit.data[0].operation.params) # would print θ[0] instead of 1.25 + + Fixed `#13478 `__. diff --git a/test/python/circuit/library/test_nlocal.py b/test/python/circuit/library/test_nlocal.py index 753feff490b1..22bfa391612d 100644 --- a/test/python/circuit/library/test_nlocal.py +++ b/test/python/circuit/library/test_nlocal.py @@ -488,6 +488,21 @@ def test_initial_state_as_circuit_object(self): self.assertCircuitEqual(ref, expected) + def test_inplace_assignment_with_cache(self): + """Test parameters are correctly re-bound in the cached gates. + + This test requires building with the Rust feature "cache_pygates" enabled, otherwise + it does not test what it is supposed to. + + Regression test of #13478. + """ + qc = EfficientSU2(2, flatten=True) + binds = [1.25] * qc.num_parameters + + qc.assign_parameters(binds, inplace=True) + bound_op = qc.data[0].operation + self.assertAlmostEqual(bound_op.params[0], binds[0]) + @ddt class TestNLocalFunction(QiskitTestCase): From 62ca8b57ffc29c86342b76770c339e3c34c67169 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 26 Nov 2024 16:56:46 +0100 Subject: [PATCH 2/4] update params on cache --- crates/circuit/src/circuit_data.rs | 16 +++++++++------- test/python/circuit/test_parameters.py | 13 +++++++++++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/crates/circuit/src/circuit_data.rs b/crates/circuit/src/circuit_data.rs index 0025664e4d94..5422138264b3 100644 --- a/crates/circuit/src/circuit_data.rs +++ b/crates/circuit/src/circuit_data.rs @@ -1361,12 +1361,9 @@ impl CircuitData { let Param::ParameterExpression(expr) = ¶ms[parameter] else { return Err(inconsistent()); }; - params[parameter] = match bind_expr( - expr.bind_borrowed(py), - ¶m_ob, - value.as_ref(), - true, - )? { + let new_param = + bind_expr(expr.bind_borrowed(py), ¶m_ob, value.as_ref(), true)?; + params[parameter] = match new_param.clone_ref(py) { Param::Obj(obj) => { return Err(CircuitError::new_err(format!( "bad type after binding for gate '{}': '{}'", @@ -1382,8 +1379,13 @@ impl CircuitData { #[cfg(feature = "cache_pygates")] { // Standard gates can all rebuild their definitions, so if the - // cached py_op exists, just clear out any existing cache. + // cached py_op exists, update the `params` attribute and clear out + // any existing cache. if let Some(borrowed) = previous.py_op.get() { + borrowed + .bind(py) + .getattr(params_attr)? + .set_item(parameter, new_param)?; borrowed.bind(py).setattr("_definition", py.None())? } } diff --git a/test/python/circuit/test_parameters.py b/test/python/circuit/test_parameters.py index e291eb415813..2b342cae1968 100644 --- a/test/python/circuit/test_parameters.py +++ b/test/python/circuit/test_parameters.py @@ -361,6 +361,19 @@ def test_assign_parameters_by_iterable(self): self.assertEqual(qc.assign_parameters(dict(zip(qc.parameters, binds)).values()), expected) self.assertEqual(qc.assign_parameters(bind for bind in binds), expected) + def test_assign_parameters_with_cache(self): + """Test assigning parameters on a circuit with already triggered cache.""" + x = Parameter("x") + qc = QuantumCircuit(1) + qc.append(RZGate(x), [0]) # add via ``append`` to create a CircuitInstruction + + _ = qc.data[0].operation.definition # trigger building the cache + + binds = [1.2] + qc.assign_parameters(binds, inplace=True) + + self.assertAlmostEqual(binds[0], qc.data[0].operation.params[0]) + def test_bind_parameters_custom_definition_global_phase(self): """Test that a custom gate with a parametrized `global_phase` is assigned correctly.""" x = Parameter("x") From 3ba81188286ee6d30c9975809a4d0a95ac851cd7 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 26 Nov 2024 16:57:13 +0100 Subject: [PATCH 3/4] Revert "Use _append_standard_gate directly" This reverts commit 9769785c11385485ef2186d7aafa6d149f4d5fa3. --- qiskit/circuit/library/n_local/n_local.py | 25 ++++++++++--------- ...l-flatten-parameters-d7eb1858e0eaa160.yaml | 18 ------------- test/python/circuit/library/test_nlocal.py | 15 ----------- 3 files changed, 13 insertions(+), 45 deletions(-) delete mode 100644 releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml diff --git a/qiskit/circuit/library/n_local/n_local.py b/qiskit/circuit/library/n_local/n_local.py index 080d84723257..8c0b4d285086 100644 --- a/qiskit/circuit/library/n_local/n_local.py +++ b/qiskit/circuit/library/n_local/n_local.py @@ -1109,11 +1109,11 @@ def _build_rotation_layer(self, circuit, param_iter, i): if k not in skipped_blocks ) for qubits in all_qubits: - circuit._append_standard_gate( - simple_block.gate, + instr = CircuitInstruction( + simple_block.gate(*itertools.islice(param_iter, simple_block.num_params)), qubits, - [next(param_iter) for _ in range(simple_block.num_params)], ) + circuit._append(instr) else: block_indices = [ list(range(k * block.num_qubits, (k + 1) * block.num_qubits)) @@ -1139,12 +1139,11 @@ def _build_entanglement_layer(self, circuit, param_iter, i): # It's actually nontrivially faster to use a listcomp and pass that to `tuple` # than to pass a generator expression directly. # pylint: disable=consider-using-generator - qubits = tuple([target_qubits[i] for i in indices]) - circuit._append_standard_gate( - simple_block.gate, - qubits, - [next(param_iter) for _ in range(simple_block.num_params)], + instr = CircuitInstruction( + simple_block.gate(*itertools.islice(param_iter, simple_block.num_params)), + tuple([target_qubits[i] for i in indices]), ) + circuit._append(instr) else: # apply the operations in the layer for indices in entangler_map: @@ -1278,6 +1277,7 @@ def get_entangler_map( _StdlibGateResult = collections.namedtuple("_StdlibGateResult", ("gate", "num_params")) +_STANDARD_GATE_MAPPING = get_standard_gate_name_mapping() def _stdlib_gate_from_simple_block(block: QuantumCircuit) -> _StdlibGateResult | None: @@ -1291,13 +1291,14 @@ def _stdlib_gate_from_simple_block(block: QuantumCircuit) -> _StdlibGateResult | if ( instruction.clbits or tuple(instruction.qubits) != tuple(block.qubits) - or getattr(instruction.operation, "_standard_gate", None) is None + or ( + getattr(_STANDARD_GATE_MAPPING.get(instruction.operation.name), "base_class", None) + is not instruction.operation.base_class + ) or tuple(instruction.operation.params) != tuple(block.parameters) ): return None - return _StdlibGateResult( - instruction.operation._standard_gate, len(instruction.operation.params) - ) + return _StdlibGateResult(instruction.operation.base_class, len(instruction.operation.params)) def _normalize_entanglement( diff --git a/releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml b/releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml deleted file mode 100644 index d7fe0e037950..000000000000 --- a/releasenotes/notes/fix-nlocal-flatten-parameters-d7eb1858e0eaa160.yaml +++ /dev/null @@ -1,18 +0,0 @@ ---- -fixes: - - | - Fixed a bug in :class:`.NLocal` and derivatives, occurring when ``flatten=True`` was set - and standard gates backed by Rust were used. When binding parameters in-place to such a - circuit, the new parameter values were not correctly propagated to the circuit instructions. - - The circuit could still be compiled and used as expected, however, inspecting the individual - operations would still show the old parameter values. For example:: - - from qiskit.circuit.library import EfficientSU2 - - circuit = EfficientSU2(2, flatten=True) - circuit.assign_parameters([1.25] * circuit.num_parameters, inplace=True) - - print(circuit.data[0].operation.params) # would print θ[0] instead of 1.25 - - Fixed `#13478 `__. diff --git a/test/python/circuit/library/test_nlocal.py b/test/python/circuit/library/test_nlocal.py index 22bfa391612d..753feff490b1 100644 --- a/test/python/circuit/library/test_nlocal.py +++ b/test/python/circuit/library/test_nlocal.py @@ -488,21 +488,6 @@ def test_initial_state_as_circuit_object(self): self.assertCircuitEqual(ref, expected) - def test_inplace_assignment_with_cache(self): - """Test parameters are correctly re-bound in the cached gates. - - This test requires building with the Rust feature "cache_pygates" enabled, otherwise - it does not test what it is supposed to. - - Regression test of #13478. - """ - qc = EfficientSU2(2, flatten=True) - binds = [1.25] * qc.num_parameters - - qc.assign_parameters(binds, inplace=True) - bound_op = qc.data[0].operation - self.assertAlmostEqual(bound_op.params[0], binds[0]) - @ddt class TestNLocalFunction(QiskitTestCase): From 1c935da6afe38c6de2aca6cafaabff3f52283103 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 26 Nov 2024 17:01:16 +0100 Subject: [PATCH 4/4] Add test and reno --- ...x-cached-params-update-4d2814b698fa76b4.yaml | 17 +++++++++++++++++ test/python/circuit/library/test_nlocal.py | 15 +++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 releasenotes/notes/fix-cached-params-update-4d2814b698fa76b4.yaml diff --git a/releasenotes/notes/fix-cached-params-update-4d2814b698fa76b4.yaml b/releasenotes/notes/fix-cached-params-update-4d2814b698fa76b4.yaml new file mode 100644 index 000000000000..a4b2b6fe6571 --- /dev/null +++ b/releasenotes/notes/fix-cached-params-update-4d2814b698fa76b4.yaml @@ -0,0 +1,17 @@ +--- +fixes: + - | + Fixed a bug in :meth:`.QuantumCircuit.assign_parameters`, occurring when assigning parameters + to standard gates whose definition has already been triggered. In this case, the new values + were not properly propagated to the gate instances. While the circuit itself was still + compiled as expected, inspecting the individual operations would still show the old parameter. + + For example:: + + from qiskit.circuit.library import EfficientSU2 + + circuit = EfficientSU2(2, flatten=True) + circuit.assign_parameters([1.25] * circuit.num_parameters, inplace=True) + print(circuit.data[0].operation.params) # would print θ[0] instead of 1.25 + + Fixed `#13478 `__. diff --git a/test/python/circuit/library/test_nlocal.py b/test/python/circuit/library/test_nlocal.py index 753feff490b1..22bfa391612d 100644 --- a/test/python/circuit/library/test_nlocal.py +++ b/test/python/circuit/library/test_nlocal.py @@ -488,6 +488,21 @@ def test_initial_state_as_circuit_object(self): self.assertCircuitEqual(ref, expected) + def test_inplace_assignment_with_cache(self): + """Test parameters are correctly re-bound in the cached gates. + + This test requires building with the Rust feature "cache_pygates" enabled, otherwise + it does not test what it is supposed to. + + Regression test of #13478. + """ + qc = EfficientSU2(2, flatten=True) + binds = [1.25] * qc.num_parameters + + qc.assign_parameters(binds, inplace=True) + bound_op = qc.data[0].operation + self.assertAlmostEqual(bound_op.params[0], binds[0]) + @ddt class TestNLocalFunction(QiskitTestCase):