From 67fd35a2eacf977f0cd2539b7ae5b4c4210f50bf Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Mon, 1 Jul 2024 16:45:52 +0200 Subject: [PATCH 1/3] Fix `replace_block_with_op` on operations with wrong number of qubits (#12637) * fix illegal op insertion * rm dangling print * fix PauliEvolution * Update qiskit/dagcircuit/dagcircuit.py Co-authored-by: John Lapeyre --------- Co-authored-by: John Lapeyre --- qiskit/dagcircuit/dagcircuit.py | 7 +++++++ .../pauli_2q_evolution_commutation.py | 6 +++++- ...n-illegal-replace-block-50cef8da757a580a.yaml | 7 +++++++ test/python/dagcircuit/test_dagcircuit.py | 16 ++++++++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/raise-on-illegal-replace-block-50cef8da757a580a.yaml diff --git a/qiskit/dagcircuit/dagcircuit.py b/qiskit/dagcircuit/dagcircuit.py index 626b7ef053ec..944e2df625b0 100644 --- a/qiskit/dagcircuit/dagcircuit.py +++ b/qiskit/dagcircuit/dagcircuit.py @@ -1342,6 +1342,13 @@ def replace_block_with_op( block_cargs.sort(key=wire_pos_map.get) new_node = DAGOpNode(op, block_qargs, block_cargs, dag=self) + # check the op to insert matches the number of qubits we put it on + if op.num_qubits != len(block_qargs): + raise DAGCircuitError( + f"Number of qubits in the replacement operation ({op.num_qubits}) is not equal to " + f"the number of qubits in the block ({len(block_qargs)})!" + ) + try: new_node._node_id = self._multi_graph.contract_nodes( block_ids, new_node, check_cycle=cycle_check diff --git a/qiskit/transpiler/passes/routing/commuting_2q_gate_routing/pauli_2q_evolution_commutation.py b/qiskit/transpiler/passes/routing/commuting_2q_gate_routing/pauli_2q_evolution_commutation.py index 641b40c9f3e1..beadc884465b 100644 --- a/qiskit/transpiler/passes/routing/commuting_2q_gate_routing/pauli_2q_evolution_commutation.py +++ b/qiskit/transpiler/passes/routing/commuting_2q_gate_routing/pauli_2q_evolution_commutation.py @@ -51,7 +51,11 @@ def run(self, dag: DAGCircuit) -> DAGCircuit: sub_dag = self._decompose_to_2q(dag, node.op) block_op = Commuting2qBlock(set(sub_dag.op_nodes())) - wire_order = {wire: idx for idx, wire in enumerate(dag.qubits)} + wire_order = { + wire: idx + for idx, wire in enumerate(sub_dag.qubits) + if wire not in sub_dag.idle_wires() + } dag.replace_block_with_op([node], block_op, wire_order) return dag diff --git a/releasenotes/notes/raise-on-illegal-replace-block-50cef8da757a580a.yaml b/releasenotes/notes/raise-on-illegal-replace-block-50cef8da757a580a.yaml new file mode 100644 index 000000000000..f4971fe520a0 --- /dev/null +++ b/releasenotes/notes/raise-on-illegal-replace-block-50cef8da757a580a.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Previously, :meth:`.DAGCircuit.replace_block_with_op` allowed to place an + ``n``-qubit operation onto a block of ``m`` qubits, leaving the DAG in an + invalid state. This behavior has been fixed, and the attempt will raise + a :class:`.DAGCircuitError`. diff --git a/test/python/dagcircuit/test_dagcircuit.py b/test/python/dagcircuit/test_dagcircuit.py index 3e4d4bf4e686..26f7e4788083 100644 --- a/test/python/dagcircuit/test_dagcircuit.py +++ b/test/python/dagcircuit/test_dagcircuit.py @@ -2903,6 +2903,22 @@ def test_single_node_block(self): self.assertEqual(expected_dag.count_ops(), dag.count_ops()) self.assertIsInstance(new_node.op, XGate) + def test_invalid_replacement_size(self): + """Test inserting an operation on a wrong number of qubits raises.""" + + # two X gates, normal circuit + qc = QuantumCircuit(2) + qc.x(range(2)) + + # mutilate the DAG + dag = circuit_to_dag(qc) + to_replace = list(dag.op_nodes()) + new_node = XGate() + idx_map = {node.qargs[0]: i for i, node in enumerate(to_replace)} + + with self.assertRaises(DAGCircuitError): + dag.replace_block_with_op(to_replace, new_node, idx_map) + def test_replace_control_flow_block(self): """Test that we can replace a block of control-flow nodes with a single one.""" body = QuantumCircuit(1) From ed87f2fa502ea0d63b072c227b5e813b6320fa56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guillermo=20Abad=20L=C3=B3pez?= <109400222+GuillermoAbadLopez@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:57:00 +0200 Subject: [PATCH 2/3] Add warning for bad `justify` input, in `circuit_drawer` (#12458) * add warning for bad justify input in circuit_drawer * change justify after raising error * typo indentation + improving warning string * Undo max_lenght limit by autoformater (120 > 105) * Make lines fullfill 105 max lenght requirement * Solve regex matching parenthesis problem and don't trigger the wanring for default value * Change justify default value to "left", & add deprecation wrapper * Change and extend warnings tests * Solve various layers of same argument DeprecationWarning * Added a clarification comment for the solution, about multiple deprecationwarnings * Ignore cyclic import error, as above * Apply suggestions from code review Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * Apply suggestions from code review * Improve DeprecationWarning readability, and fix warning checks tests * Remove `_is_valid_justify_arg` from `@deprecate_arg`, for solving circular import * in `deprecate_arg` change `since` to "1.2.0" * black formater suggestion * negate conditions for `predicate` in `@deprecate_arg` * remove `pending=True`, since then warning is not raised * change qiskit version on tests * remove assert Regex for DeprecationWarning * Add release note, and remove two undesired changes in imports * changing release note naming from "_" to "-" * Add extra line in the end, for lint * Redid release file from start, with shorter name, and correct spacings * Remove final spaces in all lines... * Try without deprecations_visualization section.. * Solve, bad Sphinx spacing, go back to deprecations_visualization * Go back to `None` as default value * Simplifying deprecation logic * Remove unused imports and changed missing default value * Improve docstring for public methods * Improve error readbility and testing of it with regex --------- Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> --- qiskit/circuit/quantumcircuit.py | 10 ++--- qiskit/visualization/circuit/_utils.py | 42 +++++++++++++++---- .../circuit/circuit_visualization.py | 23 +++++----- ...it-draw-warn-justify-03434d30cccda452.yaml | 13 ++++++ .../visualization/test_circuit_drawer.py | 24 +++++++++-- 5 files changed, 84 insertions(+), 28 deletions(-) create mode 100644 releasenotes/notes/circuit-draw-warn-justify-03434d30cccda452.yaml diff --git a/qiskit/circuit/quantumcircuit.py b/qiskit/circuit/quantumcircuit.py index 0019a15443e7..6d41e6fdcd25 100644 --- a/qiskit/circuit/quantumcircuit.py +++ b/qiskit/circuit/quantumcircuit.py @@ -3257,11 +3257,11 @@ def draw( alternative value set. For example, ``circuit_reverse_bits = True``. plot_barriers: Enable/disable drawing barriers in the output circuit. Defaults to ``True``. - justify: Options are ``left``, ``right`` or ``none``. If - anything else is supplied, it defaults to left justified. It refers - to where gates should be placed in the output circuit if there is - an option. ``none`` results in each gate being placed in its own - column. + justify: Options are ``"left"``, ``"right"`` or ``"none"`` (str). + If anything else is supplied, left justified will be used instead. + It refers to where gates should be placed in the output circuit if + there is an option. ``none`` results in each gate being placed in + its own column. Defaults to ``left``. vertical_compression: ``high``, ``medium`` or ``low``. It merges the lines generated by the `text` output so the drawing will take less vertical room. Default is ``medium``. Only used by diff --git a/qiskit/visualization/circuit/_utils.py b/qiskit/visualization/circuit/_utils.py index ca29794b9626..2077a3891542 100644 --- a/qiskit/visualization/circuit/_utils.py +++ b/qiskit/visualization/circuit/_utils.py @@ -14,21 +14,25 @@ import re from collections import OrderedDict +from warnings import warn import numpy as np from qiskit.circuit import ( + ClassicalRegister, Clbit, + ControlFlowOp, ControlledGate, Delay, Gate, Instruction, Measure, + QuantumCircuit, + Qubit, ) +from qiskit.circuit.annotated_operation import AnnotatedOperation, InverseModifier, PowerModifier from qiskit.circuit.controlflow import condition_resources from qiskit.circuit.library import PauliEvolutionGate -from qiskit.circuit import ClassicalRegister, QuantumCircuit, Qubit, ControlFlowOp -from qiskit.circuit.annotated_operation import AnnotatedOperation, InverseModifier, PowerModifier from qiskit.circuit.tools import pi_check from qiskit.converters import circuit_to_dag from qiskit.utils import optionals as _optionals @@ -370,6 +374,29 @@ def generate_latex_label(label): return final_str.replace(" ", "\\,") # Put in proper spaces +def _get_valid_justify_arg(justify): + """Returns a valid `justify` argument, warning if necessary.""" + if isinstance(justify, str): + justify = justify.lower() + + if justify is None: + justify = "left" + + if justify not in ("left", "right", "none"): + # This code should be changed to an error raise, once the deprecation is complete. + warn( + f"Setting QuantumCircuit.draw()’s or circuit_drawer()'s justify argument: {justify}, to a " + "value other than 'left', 'right', 'none' or None (='left'). Default 'left' will be used. " + "Support for invalid justify arguments is deprecated as of qiskit 1.2.0. Starting no " + "earlier than 3 months after the release date, invalid arguments will error.", + DeprecationWarning, + 2, + ) + justify = "left" + + return justify + + def _get_layered_instructions( circuit, reverse_bits=False, justify=None, idle_wires=True, wire_order=None, wire_map=None ): @@ -384,9 +411,10 @@ def _get_layered_instructions( reverse_bits (bool): If true the order of the bits in the registers is reversed. justify (str) : `left`, `right` or `none`. Defaults to `left`. Says how - the circuit should be justified. + the circuit should be justified. If an invalid value is provided, + default `left` will be used. idle_wires (bool): Include idle wires. Default is True. - wire_order (list): A list of ints that modifies the order of the bits + wire_order (list): A list of ints that modifies the order of the bits. Returns: Tuple(list,list,list): To be consumed by the visualizer directly. @@ -394,11 +422,7 @@ def _get_layered_instructions( Raises: VisualizationError: if both reverse_bits and wire_order are entered. """ - if justify: - justify = justify.lower() - - # default to left - justify = justify if justify in ("right", "none") else "left" + justify = _get_valid_justify_arg(justify) if wire_map is not None: qubits = [bit for bit in wire_map if isinstance(bit, Qubit)] diff --git a/qiskit/visualization/circuit/circuit_visualization.py b/qiskit/visualization/circuit/circuit_visualization.py index 146de9d32dee..33f6cadb46d3 100644 --- a/qiskit/visualization/circuit/circuit_visualization.py +++ b/qiskit/visualization/circuit/circuit_visualization.py @@ -28,21 +28,22 @@ import logging import os +import shutil import subprocess import tempfile -import shutil import typing from warnings import warn from qiskit import user_config -from qiskit.utils import optionals as _optionals from qiskit.circuit import ControlFlowOp, Measure +from qiskit.utils import optionals as _optionals + +from ..exceptions import VisualizationError +from ..utils import _trim as trim_image +from . import _utils from . import latex as _latex -from . import text as _text from . import matplotlib as _matplotlib -from . import _utils -from ..utils import _trim as trim_image -from ..exceptions import VisualizationError +from . import text as _text if typing.TYPE_CHECKING: from typing import Any @@ -131,11 +132,11 @@ def circuit_drawer( alternative value set. For example, ``circuit_reverse_bits = True``. plot_barriers: Enable/disable drawing barriers in the output circuit. Defaults to ``True``. - justify: Options are ``left``, ``right`` or ``none``. If - anything else is supplied, it defaults to left justified. It refers - to where gates should be placed in the output circuit if there is - an option. ``none`` results in each gate being placed in its own - column. + justify: Options are ``"left"``, ``"right"`` or ``"none"`` (str). + If anything else is supplied, left justified will be used instead. + It refers to where gates should be placed in the output circuit if + there is an option. ``none`` results in each gate being placed in + its own column. Defaults to ``left``. vertical_compression: ``high``, ``medium`` or ``low``. It merges the lines generated by the `text` output so the drawing will take less vertical room. Default is ``medium``. Only used by diff --git a/releasenotes/notes/circuit-draw-warn-justify-03434d30cccda452.yaml b/releasenotes/notes/circuit-draw-warn-justify-03434d30cccda452.yaml new file mode 100644 index 000000000000..9ae15212fd56 --- /dev/null +++ b/releasenotes/notes/circuit-draw-warn-justify-03434d30cccda452.yaml @@ -0,0 +1,13 @@ +--- +deprecations_visualization: + - | + The ``justify`` argument of :func:`circuit_drawer` or :meth:`QuantumCircuit.draw`, will + no longer support invalid values (previously changing them to the default), and in a future + release they will error. Valid justify values are ``"left"``, ``"right"`` or ``"none"``. +fixes: + - | + Fixed an issue where :func:`circuit_drawer` or the :meth:`QuantumCircuit.draw` method would + not raise a warning when an invalid value was passed to the ``justify`` argument, before + changing it to the default. Now, it will raise a warning if an invalid value is passed. + Valid justify values are ``"left"``, ``"right"`` or ``"none"``. Refer to + `#12089 ` for more details. diff --git a/test/python/visualization/test_circuit_drawer.py b/test/python/visualization/test_circuit_drawer.py index e6b430c4ee82..d459bd945046 100644 --- a/test/python/visualization/test_circuit_drawer.py +++ b/test/python/visualization/test_circuit_drawer.py @@ -14,16 +14,16 @@ import os import pathlib +import re import shutil import tempfile import unittest import warnings from unittest.mock import patch -from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister +from qiskit import ClassicalRegister, QuantumCircuit, QuantumRegister, visualization from qiskit.utils import optionals -from qiskit import visualization -from qiskit.visualization.circuit import text, styles +from qiskit.visualization.circuit import styles, text from qiskit.visualization.exceptions import VisualizationError from test import QiskitTestCase # pylint: disable=wrong-import-order @@ -241,6 +241,24 @@ def test_reverse_bits(self): result = visualization.circuit_drawer(circuit, output="text", reverse_bits=True) self.assertEqual(result.__str__(), expected) + def test_warning_for_bad_justify_argument(self): + """Test that the correct DeprecationWarning is raised when the justify parameter is badly input, + for both of the public interfaces.""" + circuit = QuantumCircuit() + bad_arg = "bad" + error_message = re.escape( + f"Setting QuantumCircuit.draw()’s or circuit_drawer()'s justify argument: {bad_arg}, to a " + "value other than 'left', 'right', 'none' or None (='left'). Default 'left' will be used. " + "Support for invalid justify arguments is deprecated as of qiskit 1.2.0. Starting no " + "earlier than 3 months after the release date, invalid arguments will error.", + ) + + with self.assertWarnsRegex(DeprecationWarning, error_message): + visualization.circuit_drawer(circuit, justify=bad_arg) + + with self.assertWarnsRegex(DeprecationWarning, error_message): + circuit.draw(justify=bad_arg) + @unittest.skipUnless(optionals.HAS_PYLATEX, "needs pylatexenc for LaTeX conversion") def test_no_explict_cregbundle(self): """Test no explicit cregbundle should not raise warnings about being disabled From 5db984ae0c33a4a8558f6a0d5394854fe1a7a2c5 Mon Sep 17 00:00:00 2001 From: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Date: Mon, 1 Jul 2024 15:01:12 -0400 Subject: [PATCH 3/3] Initial: Expose operation conversion methods to other crates. (#12698) - Expose `operation_type_to_py`, `operation_type_and_data_to_py`, `convert_py_to_operation_type`, methods to other rust crates. - Expose `OperationTypeConstruct` struct to other crates. --- crates/circuit/src/circuit_instruction.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/circuit/src/circuit_instruction.rs b/crates/circuit/src/circuit_instruction.rs index 74302b526d51..ffa6bb0c652c 100644 --- a/crates/circuit/src/circuit_instruction.rs +++ b/crates/circuit/src/circuit_instruction.rs @@ -728,10 +728,7 @@ impl CircuitInstruction { /// Take a reference to a `CircuitInstruction` and convert the operation /// inside that to a python side object. -pub(crate) fn operation_type_to_py( - py: Python, - circuit_inst: &CircuitInstruction, -) -> PyResult { +pub fn operation_type_to_py(py: Python, circuit_inst: &CircuitInstruction) -> PyResult { let (label, duration, unit, condition) = match &circuit_inst.extra_attrs { None => (None, None, None, None), Some(extra_attrs) => ( @@ -757,7 +754,7 @@ pub(crate) fn operation_type_to_py( /// a Python side full-fat Qiskit operation as a PyObject. This is typically /// used by accessor functions that need to return an operation to Qiskit, such /// as accesing `CircuitInstruction.operation`. -pub(crate) fn operation_type_and_data_to_py( +pub fn operation_type_and_data_to_py( py: Python, operation: &OperationType, params: &[Param], @@ -796,8 +793,8 @@ pub(crate) fn operation_type_and_data_to_py( /// A container struct that contains the output from the Python object to /// conversion to construct a CircuitInstruction object -#[derive(Debug)] -pub(crate) struct OperationTypeConstruct { +#[derive(Debug, Clone)] +pub struct OperationTypeConstruct { pub operation: OperationType, pub params: SmallVec<[Param; 3]>, pub label: Option, @@ -809,7 +806,7 @@ pub(crate) struct OperationTypeConstruct { /// Convert an inbound Python object for a Qiskit operation and build a rust /// representation of that operation. This will map it to appropriate variant /// of operation type based on class -pub(crate) fn convert_py_to_operation_type( +pub fn convert_py_to_operation_type( py: Python, py_op: PyObject, ) -> PyResult {