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 issues with ControlFlowOp displays #10842

Merged
merged 44 commits into from
Sep 29, 2023

Conversation

enavarro51
Copy link
Contributor

@enavarro51 enavarro51 commented Sep 14, 2023

Summary

Fixes issues with use of cregbundle option when displaying ControlFlowOps and fixes #10601

Details and comments

This PR addresses the following issues with display of ControlFlowOps with the cregbundle option

  • If a ControlFlowOp was part of a circuit being displayed in the mpl and text circuit drawers, the cregbundle option was always being set to False, no matter what the user entered.
  • Composite gates that accessed Clbit's normally force the cregbundle option to False, but if the gate was inside a ControlFlowOp, the cregbundle option was not changing.

Per discussions with @jakelishman, this PR also incorporates the fixes to #10601, previously done by @mtreinish in #10602.

In doing testing, it was also noted that the encoding option that is used by the text drawer, cannot be entered in the circuit_drawer function or in QuantumCircuit.draw meaning users would generally not have access to that option.

There were several other cleanup and efficiency changes.

Fix #10601
Close #10602 (obsolete)

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks Edwin, mostly this looks solid to me. Just a couple of minor notes.

bit_conditional_bundle.png Outdated Show resolved Hide resolved
qiskit/circuit/quantumcircuit.py Outdated Show resolved Hide resolved
qiskit/visualization/circuit/matplotlib.py Outdated Show resolved Hide resolved
qiskit/visualization/circuit/text.py Show resolved Hide resolved
qiskit/visualization/circuit/text.py Outdated Show resolved Hide resolved
@enavarro51
Copy link
Contributor Author

There were a couple of other changes in e1d7982 related to outer_circuit and wire_map that I discovered when doing #10869.

@enavarro51
Copy link
Contributor Author

1859210 fixes a bug in the for_loop test for mpl. I was seeing an about 1 out of 4 failure rate on that test. The measure inside the for loop would occasionally shift left 1 layer. I believe the problem is in _get_layered_instructions which takes the flow_wire_map as an argument. There's some tricky code in there regarding placement of measures, and I think it's currently requiring the outer wire_map to be included.

I think tracking this down into that code is beyond the scope of this PR.

@1ucian0 1ucian0 modified the milestones: 0.25.2, 0.25.3 Sep 28, 2023
@1ucian0
Copy link
Member

1ucian0 commented Sep 29, 2023

is this stable backport potential?

@jakelishman
Copy link
Member

Luciano: it is as of the resolution of #10842 (comment) - originally it had some extra bits in it. I can work through to merge this today if you'll accept it for 0.25.2 on Monday - I've heard from a couple of people that the bug being fixed here would be important for them.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for this Edwin - I've fixed up the merge conflicts, and this looks good to merge now.

There'll be a minor conflict when we try to backport this, but I'll fix that up manually.

@jakelishman jakelishman added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Sep 29, 2023
@jakelishman jakelishman added this pull request to the merge queue Sep 29, 2023
Merged via the queue into Qiskit:main with commit 404df43 Sep 29, 2023
14 checks passed
mergify bot pushed a commit that referenced this pull request Sep 29, 2023
* Fix override bug and testing

* Preliminary changes

* First attempts

* Work on x_index

* Early testing

* Cleanup getattr

* Complete adding exprs

* First fix for cregbundle control flow

* Finish basic cregbundle mods

* Finish all cregbundle except wire_map bug

* Move carg check to circuit_vis and change text tests to circuit_drawer

* Move mpl tests to use circuit_drawer and update images for cregbundle changes

* Change mpl tests to circuit_drawer

* Fix flow_wire_map and tranpile bug

* Final cleanup

* Missing outputs on mpl tests

* Fix output

* Restore tests and move carg check to text and mpl and make circuit

* Prepare final cregbundle fix

* Update ref images

* Fixing refs

* Update refs again

* Final mods

* Incorporate mtreinish changes

* Add reno

* Fix if_else_op test

* Lint

* Fix target == expr bug in text

* Review fixes and outer_circuit, wire_map fixes

* Remove png

* Minor changes

* Fix for_loop measure placement bug

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
(cherry picked from commit 404df43)

# Conflicts:
#	qiskit/visualization/circuit/text.py
#	test/python/visualization/test_circuit_text_drawer.py
#	test/visual/mpl/circuit/test_circuit_matplotlib_drawer.py
jakelishman added a commit that referenced this pull request Sep 29, 2023
Cherry-picked version of only the MPL-drawer changes from the given
commit, since the text-drawer support for dynamic circuits is not in the
old version.

(cherry picked from commit 404df43)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
jakelishman added a commit that referenced this pull request Sep 29, 2023
Cherry-picked version of only the MPL-drawer changes from the given
commit, since the text-drawer support for dynamic circuits is not in the
old version.

(cherry picked from commit 404df43)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
github-merge-queue bot pushed a commit that referenced this pull request Sep 29, 2023
Cherry-picked version of only the MPL-drawer changes from the given
commit, since the text-drawer support for dynamic circuits is not in the
old version.

(cherry picked from commit 404df43)

Co-authored-by: Edwin Navarro <enavarro@comcast.net>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
rupeshknn pushed a commit to rupeshknn/qiskit that referenced this pull request Oct 9, 2023
* Fix override bug and testing

* Preliminary changes

* First attempts

* Work on x_index

* Early testing

* Cleanup getattr

* Complete adding exprs

* First fix for cregbundle control flow

* Finish basic cregbundle mods

* Finish all cregbundle except wire_map bug

* Move carg check to circuit_vis and change text tests to circuit_drawer

* Move mpl tests to use circuit_drawer and update images for cregbundle changes

* Change mpl tests to circuit_drawer

* Fix flow_wire_map and tranpile bug

* Final cleanup

* Missing outputs on mpl tests

* Fix output

* Restore tests and move carg check to text and mpl and make circuit

* Prepare final cregbundle fix

* Update ref images

* Fixing refs

* Update refs again

* Final mods

* Incorporate mtreinish changes

* Add reno

* Fix if_else_op test

* Lint

* Fix target == expr bug in text

* Review fixes and outer_circuit, wire_map fixes

* Remove png

* Minor changes

* Fix for_loop measure placement bug

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: visualization qiskit.visualization stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control flow mpl visualization not respecting layout
4 participants