Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix circuit drawer for instructions with circuit parameters #9942

Merged
merged 10 commits into from
Apr 20, 2023
12 changes: 6 additions & 6 deletions qiskit/visualization/circuit/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,9 @@
Gate,
Instruction,
Measure,
ControlFlowOp,
)
from qiskit.circuit.library import PauliEvolutionGate
from qiskit.circuit import ClassicalRegister
from qiskit.circuit import ClassicalRegister, QuantumCircuit
from qiskit.circuit.tools import pi_check
from qiskit.converters import circuit_to_dag
from qiskit.utils import optionals as _optionals
Expand Down Expand Up @@ -119,10 +118,11 @@ def get_gate_ctrl_text(op, drawer, style=None, calibrations=None):

def get_param_str(op, drawer, ndigits=3):
"""Get the params as a string to add to the gate text display"""
if not hasattr(op, "params") or any(isinstance(param, np.ndarray) for param in op.params):
return ""

if isinstance(op, ControlFlowOp):
if (
not hasattr(op, "params")
or any(isinstance(param, np.ndarray) for param in op.params)
or any(isinstance(param, QuantumCircuit) for param in op.params)
):
Comment on lines +121 to +125
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine. It would be good to add tests for these 3 cases. Though the code is in utils.py, the easiest place to test it would probably be in test_circuit_text_drawer. Also a simple release note indicating that a QuantumCircuit as a parameter will no longer display in the circuit drawers due to formatting issues.

Copy link
Contributor Author

@diemilio diemilio Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much @enavarro51. I wasn't sure if additional tests were needed since this code was a minor change for something I thought was already being tested.

Also, thanks for pointing out where the tests should go. I will look into test_circuit_text_drawer and add them. Just to double check, are tests only for the text drawer enough? Or do other drawers (like mpl) need them too?

I'll add the reno as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there apparently aren't any tests for these 3 options. The text drawer is sufficient. You're really just checking the call to _get_param_str returns "". You could do that in test_utils if you want, but I think it's nice to have a visual output for visualizations.

return ""

if isinstance(op, Delay):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fixes:
- |
Fixes the circuit drawer from displaying parameters when they are of type :class:`.QuantumCircuit`
since this resulted in an illegible drawing.

50 changes: 49 additions & 1 deletion test/python/visualization/test_circuit_text_drawer.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import numpy

from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister, transpile
from qiskit.circuit import Gate, Parameter, Qubit, Clbit
from qiskit.circuit import Gate, Parameter, Qubit, Clbit, Instruction
from qiskit.quantum_info.operators import SuperOp
from qiskit.quantum_info.random import random_unitary
from qiskit.test import QiskitTestCase
Expand Down Expand Up @@ -1839,6 +1839,21 @@ def test_control_gate_label_with_cond_2_low_cregbundle(self):
class TestTextDrawerParams(QiskitTestCase):
"""Test drawing parameters."""

def test_text_no_parameters(self):
"""Test drawing with no parameters"""
expected = "\n".join(
[
" ┌───┐",
"q: |0>┤ X ├",
" └───┘",
]
)

qr = QuantumRegister(1, "q")
circuit = QuantumCircuit(qr)
circuit.x(0)
self.assertEqual(str(_text_circuit_drawer(circuit)), expected)

def test_text_parameters_mix(self):
"""cu3 drawing with parameters"""
expected = "\n".join(
Expand Down Expand Up @@ -1904,6 +1919,39 @@ def test_text_utf8(self):
circuit.u(0, phi, lam, 0)
self.assertEqual(circuit.draw(output="text").single_string(), expected)

def test_text_ndarray_parameters(self):
"""Test that if params are type ndarray, params are not displayed."""
# fmt: off
expected = "\n".join([" ┌─────────┐",
"q: |0>┤ Unitary ├",
" └─────────┘"])
# fmt: on
qr = QuantumRegister(1, "q")
circuit = QuantumCircuit(qr)
circuit.unitary(numpy.array([[0, 1], [1, 0]]), 0)
self.assertEqual(str(_text_circuit_drawer(circuit)), expected)

def test_text_qc_parameters(self):
"""Test that if params are type QuantumCircuit, params are not displayed."""
expected = "\n".join(
[
" ┌───────┐",
"q_0: |0>┤0 ├",
" │ name │",
"q_1: |0>┤1 ├",
" └───────┘",
]
)

my_qc_param = QuantumCircuit(2)
my_qc_param.h(0)
my_qc_param.cx(0, 1)
inst = Instruction("name", 2, 0, [my_qc_param])
qr = QuantumRegister(2, "q")
circuit = QuantumCircuit(qr)
circuit.append(inst, [0, 1])
self.assertEqual(str(_text_circuit_drawer(circuit)), expected)


class TestTextDrawerVerticalCompressionLow(QiskitTestCase):
"""Test vertical_compression='low'"""
Expand Down