Add `draw` method to `Operator` class #10271

diemilio · 2023-06-13T11:31:34Z

Summary

Details and comments

Add a draw method for the Operator class. This PR covers the text, latex_source and latex, whose behavior is equivalent to that of the Statevector and DensityMatrix classes.

qiskit-bot · 2023-06-13T11:31:39Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@enavarro51
@Qiskit/terra-core
@ikkoham

diemilio · 2023-06-13T11:36:36Z

Hi @jakelishman,

Thanks so much for your feedback yesterday. I am opening the PR as you recommended. I think my only concern is what I mentioned here regarding testing:

The tests for the draw method in the Statevector and DensityMatrix classes do not have any assertions (see here, and here). They just call each of the drawers. I did the same for the Operator class, but not sure if this needs to be fixed.

ikkoham

Thanks. I left some comments about the code.

qiskit/quantum_info/operators/operator.py

ikkoham · 2023-06-14T13:10:41Z

qiskit/quantum_info/operators/operator.py

+            output = default_output
+
+        if output == "repr":
+            return "self.__repr__()"


Suggested change

return "self.__repr__()"

return self.__repr__()

Is this correct?

@ikkoham, sorry, this is definitely a mistake 😓. I will fix it. I think this shows that the tests definitely need to be fixed as I mentioned in this comment. Do you have any suggestions for what to test for? Also, do you know how we could test the latex option? I wanted to follow what had been done for the Statevector and DensityMatrix drawers, but I think those tests also need to be fixed.

hmm... testing latex is difficult. Sorry. I do not have good idea.

ikkoham · 2023-06-14T13:12:39Z

qiskit/quantum_info/operators/operator.py

+
+        else:
+            raise ValueError(
+                """'{}' is not a valid option for drawing {} objects. Please choose from:


f-string is better than format

Sounds good! I will change it. I was trying to match the code of what's done for states, which uses format.

coveralls · 2023-06-19T18:14:22Z

Pull Request Test Coverage Report for Build 5728161174

16 of 28 (57.14%) changed or added relevant lines in 2 files are covered.
8 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.01%) to 85.91%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/visualization/state_visualization.py	2	7	28.57%
qiskit/quantum_info/operators/operator.py	14	21	66.67%

Files with Coverage Reduction	New Missed Lines	%
qiskit/visualization/state_visualization.py	1	56.95%
qiskit/pulse/library/waveform.py	3	93.75%
crates/qasm2/src/lex.rs	4	92.15%

Totals
Change from base Build 5727764372:	0.01%
Covered Lines:	73098
Relevant Lines:	85087

💛 - Coveralls

diemilio · 2023-07-05T23:36:01Z

Hi @ikkoham. Do you have any other comments/changes regarding this PR? Happy to take care of anything that might be missing.

jakelishman

Thanks for looking at this Diego, and sorry I left it so long. I only had a couple of minor things, mostly about how to fix the build.

qiskit/quantum_info/operators/operator.py

jakelishman · 2023-08-03T10:55:47Z

qiskit/visualization/state_visualization.py

-            if set(state.dims()) == {2}:
+            if isinstance(state, (Statevector, DensityMatrix)) and set(state.dims()) == {2}:
                dims = False
            else:
                dims = True


The same logical suppression here would be in the Operator is a square multi-qubit operator, i.e. len(op.dims_l) == len(op.dims_r) and set(op.dims_l) == set(op.dims_r) == {2} (or whatever the syntax is for getting the left- and right-dimensions).

jakelishman · 2023-08-03T10:58:05Z

qiskit/visualization/state_visualization.py

-            dimstr += f"dims={self.state._op_shape.dims_l()}"
+            if isinstance(self.state, (Statevector, DensityMatrix)):
+                dimstr += f"dims={self.state._op_shape.dims_l()}"
+            elif isinstance(self.state, Operator):
+                dimstr += f"input_dims={self.state.input_dims()}, "
+                dimstr += f"output_dims={self.state.output_dims()}"
+


I always get a little nervous when I see unhandled else branches like this, because if the interface were to expand in the future, we'd silently do nothing. Maybe just drop the isinstance(self.state, Operator) check and make it an unconditional else? That way at least, if the interface expands in the future, we'll at least either do something sensible, or raise an error - both are better than silently doing nothing.

jakelishman · 2023-08-03T10:59:36Z

releasenotes/notes/add-operator-draw-method-47f0381ba3f2a280.yaml

+    The :class:`Operator` class now has ``.draw()`` methods allowing it to be displayed as 
+    a text matrix, IPython Latex object or Latex source. The default draw type still is the 
+    ASCII `__repr__` of the operator.


Suggested change

The :class:`Operator` class now has ``.draw()`` methods allowing it to be displayed as

a text matrix, IPython Latex object or Latex source. The default draw type still is the

ASCII `__repr__` of the operator.

The :class:`Operator` class now has a :meth:`~.Operator.draw` method allowing it to be displayed as

a text matrix, IPython LaTeX object or LaTeX source. The default draw type still is the

ASCII ``__repr__`` of the operator.

jakelishman · 2023-08-03T11:00:29Z

releasenotes/notes/add-operator-draw-method-47f0381ba3f2a280.yaml

+    .. jupyter-execute::
+
+            from qiskit.quantum_info import Operator
+            op = Operator.from_label('XH')
+            op.draw('latex')


We don't use jupyter-execute in the docs, which is why the build is failing (we found it to be too unreliable and too slow). In this case, I think it's probably ok just to skip the example.

jakelishman · 2023-08-03T11:01:51Z

test/python/quantum_info/operators/test_operator.py

+    def test_drawings(self):
+        """Test draw method"""
+        qc1 = QFT(5)
+        op = Operator.from_circuit(qc1)
+        with self.subTest(msg="str(operator)"):
+            str(op)
+        for drawtype in ["repr", "text", "latex_source"]:
+            with self.subTest(msg=f"draw('{drawtype}')"):
+                op.draw(drawtype)
+        with self.subTest(msg=" draw('latex')"):
+            op.draw("latex")


I see that this test is the same as for Statevector and DensityMatrix, so it's probably best to leave it as you've written it (i.e. just assert that no errors are raised), though if there were something sensible we could sensibly assert about the output, that wouldn't hurt.

diemilio · 2023-08-03T21:17:52Z

Thank you so much for taking the time to look into this, @jakelishman. I made all the changes you recommended, but please let me know if you feel there is anything missing.

jakelishman

Looks good to me now, thanks for the updates and sorry it took so long to get final review / merge!

diemilio · 2023-08-04T17:51:02Z

No prob. Thank you @jakelishman for the feedback.

* add draw method to Operator class * add _ipython_display_ to operator class * (fix) check type of state in TextMatrix to allow the use of Operators * add Operator drawer tests * eblack, elint changes * add reno * (fix) replace repr output, change ValueError to f-string * (fix) remove duplicate 'latex' option in test_operator * fix lint, build errors * add condition to TextMatrix to hide dims for qubit operator * (fix) remove elif statement in TextMatrix to avoid unhandled else branch

diemilio added 8 commits May 26, 2023 11:10

add draw method to Operator class

61d52de

add _ipython_display_ to operator class

fca30eb

(fix) check type of state in TextMatrix to allow the use of Operators

d2e18b8

add Operator drawer tests

d6910d9

eblack, elint changes

4bab453

@merge branch 'main' into add-operator-draw

3dc1d53

add reno

206ffe9

Merge branch 'main' into add-operator-draw

8566ca9

diemilio requested review from a team, nonhermitian and ikkoham as code owners June 13, 2023 11:31

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 13, 2023

ikkoham reviewed Jun 15, 2023

View reviewed changes

(fix) replace repr output, change ValueError to f-string

7f8914a

(fix) remove duplicate 'latex' option in test_operator

9d23980

Merge branch 'main' into add-operator-draw

69b7524

jakelishman reviewed Aug 3, 2023

View reviewed changes

diemilio added 4 commits August 3, 2023 10:36

fix lint, build errors

6c0c614

add condition to TextMatrix to hide dims for qubit operator

163ff83

(fix) remove elif statement in TextMatrix to avoid unhandled else branch

f6b205e

Merge branch 'main' into add-operator-draw

cfe492e

jakelishman approved these changes Aug 4, 2023

View reviewed changes

jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: quantum info Related to the Quantum Info module (States & Operators) mod: visualization qiskit.visualization labels Aug 4, 2023

jakelishman added this to the 0.45.0 milestone Aug 4, 2023

jakelishman added this pull request to the merge queue Aug 4, 2023

Merged via the queue into Qiskit:main with commit d7f02a6 Aug 4, 2023

Add draw method to Operator class #10271

Add draw method to Operator class #10271

Uh oh!

Conversation

diemilio commented Jun 13, 2023 • edited by jakelishman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Jun 13, 2023

Uh oh!

diemilio commented Jun 13, 2023

Uh oh!

ikkoham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diemilio Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 5728161174

💛 - Coveralls

Uh oh!

diemilio commented Jul 5, 2023

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diemilio commented Aug 3, 2023

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

diemilio commented Aug 4, 2023

Uh oh!

Uh oh!

Add `draw` method to `Operator` class #10271

Add `draw` method to `Operator` class #10271

diemilio commented Jun 13, 2023 •

edited by jakelishman

Loading

diemilio Jun 15, 2023 •

edited

Loading

coveralls commented Jun 19, 2023 •

edited

Loading