Fix circuit drawer for instructions with circuit parameters #9942

diemilio · 2023-04-11T13:49:20Z

Summary

Fixes #9908. For instructions containing circuits as parameters, the circuit drawer includes the gate names (and their corresponding parameters), making the drawing illegible. This PR addresses this issue by adding a conditional statement to ignore adding the parameters to the drawing if they are of type QuantumCircuit.

Details and comments

Adding this condition also allowed removing the separate if statement needed for ControlFlowOp, which had a similar issue.

…arams of type QuantumCircuit

qiskit-bot · 2023-04-11T13:49:25Z

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:

@Qiskit/terra-core
@enavarro51

coveralls · 2023-04-11T14:18:09Z

Pull Request Test Coverage Report for Build 4748219270

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
24 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.0005%) to 85.906%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	95.37%
crates/accelerate/src/sabre_swap/layer.rs	4	97.32%
crates/qasm2/src/lex.rs	6	90.38%
crates/qasm2/src/parse.rs	12	96.65%

Totals
Change from base Build 4744407668:	0.0005%
Covered Lines:	71014
Relevant Lines:	82665

💛 - Coveralls

enavarro51 · 2023-04-12T15:11:07Z

qiskit/visualization/circuit/_utils.py

+    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)
+    ):


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.

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.

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.

Merge branch 'main'

…e QuantumCircuit

diemilio · 2023-04-17T13:41:38Z

Hi @enavarro51. I added a test to check that the drawer does not display parameters when they are of type QuantumCircuit. However, I had a few questions:

I added the test under the TestTextDrawerParams class. I hope that's OK, but let me know if it should go somewhere else.
The test I added passes a qc to the parameters of an Instruction, which then gets appended to another quantum circuit (similar to what was described in Circuit drawer is illegible for instructions with circuit parameters. #9908). This test works ok, but another way to test for it would be to check that the drawer is working correctly for ControlFlowOp objects such as IfElseOp or ForLoopOp, for which there are no tests at this point. Let me know if what I added is enough, or if you think tests for some of the ControlFlowOp.
I am a bit confused about the test for parameters of type ndarray. Wouldn't the drawer have the same issue if the parameters are of type list? Or is there a specific case where numpy arrays are being used? It would be good to know so I can add a test that resembles what this condition was originally added for. I tried looking at the commit history to see why this check was added, but unfortunately it only takes me back to when the whole python file was moved from some other location.
Do we need a separate test for not hasattr(op, "params")? Aren't we checking for this condition every time we draw something that does not have params?

Thanks! and sorry for having so many questions.

enavarro51 · 2023-04-17T23:39:33Z

That's a good place
Adding the circuit manually covers the ControlFlowOp case. I think that's enough for now. There's work currently being done to display these ops with their circuits in the drawers and tests will need to be added then.
Theoretically the drawers should handle a list of any number of params,, though this might make for some very wide gates. The problem with the ndarray is that it can be multi-dimensional and some gates like Hamiltonian use ndarray automatically. So we should test for this. It can be a simple array.
For my thinking, it's probably best to add such a test, since if something were to change in the future, depending on default behavior not be optimal.

Also I think you need to run black qiskit test tools. Thanks for everything.

jakelishman · 2023-04-17T23:50:53Z

About ndarray parameters: there are a few old Qiskit gates that specifically use an ndarray as a parameter (Initialize in statevector mode and UnitaryGate storing its matrix come to mind). There's no built-in Qiskit gates that use a list, and by default, all instructions would reject setting an element of params to be a list. If we're going the route of explicit blocking of certain parameters, then both ndarray and QuantumCircuit parameters should be skipped (like you've done here). There's no need to add an extra test of ndarray parameters - it should be already being tested by one of the two things I mentioned.

diemilio · 2023-04-18T18:42:29Z

Hi @jakelishman. Sorry, I am bit confused now. Should I implement a test for the ndarray case? It seems test_unitary_nottogether_across_4 would cover this already, but it sounds like @enavarro51 would still like to add a separate test. I doesn't really take too much time to do this, so please let me know what would you prefer. Also, there's two ways I could go about it, using a unitary gate with some ndarray as you mentioned, or passing an ndarray as a param to an instruction and then appending it to a circuit. Both are equally easy to implement, so if you decide we need to add the test, please let me know what's more appropriate.

@enavarro51, for the not hasattr(op, "params") case, I am not quite sure what would be an appropriate test other than just adding an non-parametrized gate to a circuit. Did you have something more specific in mind?

jakelishman · 2023-04-18T18:51:14Z

Oh my bad, I'd misread what Edwin said. Adding a direct test with a name like "test ndarray parameter" absolutely wouldn't hurt - let's just do that. It should already be covered as you say, but at the moment, I think it's just covered coincidentally rather than having a specific unit test for it.

enavarro51 · 2023-04-18T22:33:29Z

The not hasattr test can be that simple. It's just good to have tests all in one place for code that's in one place.

…darray

Merge branch 'main' into fix-drawer-for-instr

diemilio · 2023-04-19T19:19:46Z

@enavarro51, I added the extra tests and the release notes. Please let me know if you would like for me to change/add anything.

Thanks so much for your feedback and your help.

enavarro51

Overall LGTM. Thanks for taking this on.

enavarro51 · 2023-04-19T20:52:56Z

releasenotes/notes/fix-circuit-drawer-for-qc-params-e78c67310ae43ccf.yaml

+fixes:
+  - |
+    Fixes the circuit drawer from displaying parameters when they are of type `QuantumCircuit` 
+    since this resulted in an illegible drawing.


Just a small thing. Release notes should have a blank line at the end. The lint catcher didn't get it, but it might in a docs build.

Just to clarify - it's just a trailing newline that's missing, though if your editor doesn't add them automatically I guess it probably appears as a blank line. I don't think it will cause any failures in the build, but I could be wrong.

jakelishman

Minor tweak to the release note aside, this looks good to me. Thanks for all the work, and thanks Edwin for helping guide this PR to the end as well.

jakelishman · 2023-04-19T21:11:28Z

releasenotes/notes/fix-circuit-drawer-for-qc-params-e78c67310ae43ccf.yaml

@@ -0,0 +1,4 @@
+fixes:
+  - |
+    Fixes the circuit drawer from displaying parameters when they are of type `QuantumCircuit` 


Suggested change

Fixes the circuit drawer from displaying parameters when they are of type `QuantumCircuit`

Fixes the circuit drawer from displaying parameters when they are of type :class:`.QuantumCircuit`

this makes it a Sphinx cross-reference, so the link will be clickable

jakelishman · 2023-04-19T21:12:27Z

releasenotes/notes/fix-circuit-drawer-for-qc-params-e78c67310ae43ccf.yaml

+fixes:
+  - |
+    Fixes the circuit drawer from displaying parameters when they are of type `QuantumCircuit` 
+    since this resulted in an illegible drawing.


Just to clarify - it's just a trailing newline that's missing, though if your editor doesn't add them automatically I guess it probably appears as a blank line. I don't think it will cause any failures in the build, but I could be wrong.

diemilio · 2023-04-19T21:50:52Z

Thanks so much @enavarro51 and @jakelishman, this is good to know. I have made the changes.

jakelishman

Thanks for this!

) * fix: modify get_param_str to prevent circuit drawer from displaying params of type QuantumCircuit * fix lint errors * test: add circuit drawer test for instructions with parameters of type QuantumCircuit * test: add circuit drawer tests for no parameters and params of type ndarray * fix lint errors * remove variable to fix lint error * add reno * update releasenotes/notes/fix-circuit-drawer-for-qc-params

diemilio added 2 commits April 9, 2023 09:44

fix: modify get_param_str to prevent circuit drawer from displaying p…

af823bb

…arams of type QuantumCircuit

fix lint errors

9e966e0

diemilio requested review from a team and nonhermitian as code owners April 11, 2023 13:49

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

enavarro51 reviewed Apr 12, 2023

View reviewed changes

diemilio added 2 commits April 14, 2023 08:29

Merge branch 'main' into fix-drawer-for-instr

b9111cd

Merge branch 'main'

test: add circuit drawer test for instructions with parameters of typ…

6624604

…e QuantumCircuit

diemilio added 5 commits April 19, 2023 11:09

test: add circuit drawer tests for no parameters and params of type n…

48d4465

…darray

fix lint errors

9c1d40a

remove variable to fix lint error

7fd576c

add reno

fd6ad69

Merge branch 'main' into fix-drawer-for-instr

50bcc2e

Merge branch 'main' into fix-drawer-for-instr

enavarro51 reviewed Apr 19, 2023

View reviewed changes

jakelishman reviewed Apr 19, 2023

View reviewed changes

jakelishman added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: visualization qiskit.visualization labels Apr 19, 2023

update releasenotes/notes/fix-circuit-drawer-for-qc-params

5feaf89

jakelishman approved these changes Apr 20, 2023

View reviewed changes

jakelishman added this pull request to the merge queue Apr 20, 2023

Merged via the queue into Qiskit:main with commit 33704ea Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix circuit drawer for instructions with circuit parameters #9942

Fix circuit drawer for instructions with circuit parameters #9942

diemilio commented Apr 11, 2023

qiskit-bot commented Apr 11, 2023

coveralls commented Apr 11, 2023 •

edited

Loading

enavarro51 Apr 12, 2023

diemilio Apr 12, 2023 •

edited

Loading

enavarro51 Apr 12, 2023

diemilio commented Apr 17, 2023

enavarro51 commented Apr 17, 2023

jakelishman commented Apr 17, 2023

diemilio commented Apr 18, 2023

jakelishman commented Apr 18, 2023

enavarro51 commented Apr 18, 2023

diemilio commented Apr 19, 2023

enavarro51 left a comment

enavarro51 Apr 19, 2023

jakelishman Apr 19, 2023

jakelishman left a comment

jakelishman Apr 19, 2023

jakelishman Apr 19, 2023

diemilio commented Apr 19, 2023

jakelishman left a comment

	Fixes the circuit drawer from displaying parameters when they are of type `QuantumCircuit`
	Fixes the circuit drawer from displaying parameters when they are of type :class:`.QuantumCircuit`

Fix circuit drawer for instructions with circuit parameters #9942

Fix circuit drawer for instructions with circuit parameters #9942

Conversation

diemilio commented Apr 11, 2023

Summary

Details and comments

qiskit-bot commented Apr 11, 2023

coveralls commented Apr 11, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4748219270

💛 - Coveralls

enavarro51 Apr 12, 2023

Choose a reason for hiding this comment

diemilio Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

enavarro51 Apr 12, 2023

Choose a reason for hiding this comment

diemilio commented Apr 17, 2023

enavarro51 commented Apr 17, 2023

jakelishman commented Apr 17, 2023

diemilio commented Apr 18, 2023

jakelishman commented Apr 18, 2023

enavarro51 commented Apr 18, 2023

diemilio commented Apr 19, 2023

enavarro51 left a comment

Choose a reason for hiding this comment

enavarro51 Apr 19, 2023

Choose a reason for hiding this comment

jakelishman Apr 19, 2023

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Apr 19, 2023

Choose a reason for hiding this comment

jakelishman Apr 19, 2023

Choose a reason for hiding this comment

diemilio commented Apr 19, 2023

jakelishman left a comment

Choose a reason for hiding this comment

coveralls commented Apr 11, 2023 •

edited

Loading

diemilio Apr 12, 2023 •

edited

Loading