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

Submitting a non-string label to instructions leads to a TypeError when a QuantumCircuit instance is drawn. #7667

Closed
upsideon opened this issue Feb 16, 2022 · 1 comment · Fixed by #7671
Assignees
Labels
bug Something isn't working

Comments

@upsideon
Copy link
Contributor

upsideon commented Feb 16, 2022

Environment

  • Qiskit Terra version: 0.19.2
  • Python version: 3.8.8
  • Operating system: Pop!_OS 21.10

What is happening?

While applying an X gate to two qubits, I made a mistake in which instead of passing in the target qubit indices as a list like so, qc.x([0, 1]), I submitted them as separate parameters as in, qc.x(0, 1). This lead to the following exception when I later called qc.draw:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/text.py", line 657, in __repr__
    return self.single_string()
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/text.py", line 665, in single_string
    return "\n".join(self.lines()).encode(self.encoding).decode(self.encoding)
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/text.py", line 709, in lines
    layers = self.build_layers()
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/text.py", line 1138, in build_layers
    layer, current_connections, connection_label = self._node_to_gate(node, layer)
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/text.py", line 1005, in _node_to_gate
    gate_text = gate_text + params
TypeError: unsupported operand type(s) for +: 'int' and 'str'

I quickly realized that I had failed to pass in the indices as a list, but was puzzled as to why I was getting this exception. I took a look at the documentation corresponding to x and saw that the second index had been stored in the optional label parameter. This results in a TypeError being raised here when the instruction is processed during a draw.

Note that a similar error arises, when one attempts to draw the circuit using Matplotlib, so this is not isolated to text based circuit displays:

Traceback (most recent call last):
  File "error.py", line 6, in <module>
    qc.draw(output='mpl')
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/circuit/quantumcircuit.py", line 1865, in draw
    return circuit_drawer(
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/circuit_visualization.py", line 284, in circuit_drawer
    image = _matplotlib_circuit_drawer(
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/circuit_visualization.py", line 678, in _matplotlib_circuit_drawer
    return qcd.draw(filename)
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/matplotlib.py", line 278, in draw
    self._get_layer_widths()
  File "/home/username/anaconda3/lib/python3.8/site-packages/qiskit/visualization/matplotlib.py", line 394, in _get_layer_widths
    and len(gate_text) < 3
TypeError: object of type 'int' has no len()

How can we reproduce the issue?

This issue can be produced by executing the following code:

from qiskit import QuantumCircuit

qc = QuantumCircuit(2)
qc.x(0, 1)
print(qc)

Note that here I use a print instead of a draw to demonstrate that the same code is touched on either path.

What should happen?

As a Qiskit user, I would expect an exception to be raised when I attempt to create an instruction while providing a label with an invalid type, rather than when I try to draw it. Currently, I receive an exception, but it's distanced from the root cause and isn't as clear or clean as a ValueError raised on instruction creation would be.

Any suggestions?

I'm new to Qiskit, so forgive me if my suggestions are off base, but I did some sleuthing and determined a possible fix. Looking through the code, I found that all instructions have Instruction as their base class. If we were to add a type check to the section of Instruction's constructor which sets the optional label when present, we could raise a ValueError that would notify the user of their mistake as soon as the instruction is created. I have a draft of such a fix working locally and would love to submit a pull request, if the group would like to assign this issue to me. Even if we don't go with my suggested fix, my willingness to help out remains.

@upsideon upsideon added the bug Something isn't working label Feb 16, 2022
@jakelishman
Copy link
Member

jakelishman commented Feb 16, 2022

We need to be a little cautious when dealing with our inner-most objects that we don't introduce performance regressions in the name of error checking, because in Python we can never type-check exactly (the user can always screw with it if they try), and users are meant to read the docs to pass the correct thing.

That said, I think this is a fairly straightforwards quality-of-life improvement, and we only need to pay a (very very small) penalty if the label is actually changed from None, which is exceptionally rare in practice. I'd be happy to accept a PR on this front - I'll assign you thanks! One thing I would say is that this is a TypeError, not a ValueError - TypeError is if a given object isn't the correct type for an operation, and ValueError is for if it's the right type, but an incorrect value (like passing a negative number to a function that needs positive numbers).

@mergify mergify bot closed this as completed in #7671 Apr 18, 2022
mergify bot pushed a commit that referenced this issue Apr 18, 2022
…ion constructor. (#7671)

* Fixes #7667 by ensuring that instruction labels are strings when provided to the Instruction constructor.

* Optimizing speed of instruction label type checking.

* Switching to an equality rather than reference check for instruction labels.

* Adding a release note for label type checking on creation.

* Use Sphinx cross-ref in release note

* Fix release note

* Add ctrl_state to non-parameter kwargs

Co-authored-by: Jake Lishman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants