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 issue with ControlFlowOps and cregbundle and mods to circuit drawer testing #10804

Closed
wants to merge 24 commits into from

Conversation

enavarro51
Copy link
Contributor

Summary

Fixes an issue where use of ControlFlowOps would force cregbundle to False.
Changes function calls in "mpl" and "text" drawer tests.

Details and comments

When #10207 and #10414 were done to add display of internal circuits in ControlFlowOps to the mpl and text drawers, the drawings did not properly handle cregbundle=True. This was due to a check for cargs that inadvertently affected ControlFlowOps. In addition, the check was not looking inside the circuits of ControlFlowOps which meant the check could miss composite gates with cargs that should be setting cregbundle=False.

There was a similar cregbundle check due to the use of wire_order that was done at the beginning of the circuit_drawer function. It was appropriate to put the cargs check there as well, but both of these checks create a problem for the mpl and text test suites. test_circuit_text_drawer.py uses _text_circuit_drawer which is a subsidiary function to circuit_drawer. This means the 2 checks described above never get done during the testing, which can cause incorrect output. Similarly for mpl tests which use _matplotlib_circuit_drawer for testing.

This also means we are not really testing the user experience since they use QuantumCircuit.draw or circuit_drawer to display circuits. The solution was to convert the tests suites to use circuit_drawer instead of the subsidiary functions.

It was also discovered that the encoding option for _text_circuit_drawer did not exist in circuit_drawer which meant users could not set it. This was added to circuit_drawer and QuantumCircuit.draw.

In fixing the cregbundle issue in the mpl drawer, it was discovered that the condition lines could, for larger numbers of bits in a creg, overlay the cregbundle info. Minor adjustments were made to the cregbundle display and this is the reason for the extra updated mpl reference images.

A few consistency and cleanup changes left over from #10207 and #10414 were done as well.

@enavarro51 enavarro51 requested review from nonhermitian and a team as code owners September 9, 2023 21:47
@qiskit-bot
Copy link
Collaborator

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

@1ucian0 1ucian0 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Sep 22, 2023
@1ucian0 1ucian0 added this to the 0.25.2 milestone Sep 22, 2023
@1ucian0 1ucian0 modified the milestones: 0.25.2, 0.45.0 Sep 22, 2023
@1ucian0
Copy link
Member

1ucian0 commented Sep 22, 2023

Based on the amount of changes, I would consider this bugfix for next stable, instead of patch release. But I dont feel super strongly about it.

@jakelishman
Copy link
Member

Was this replaced by #10842 and #10869? I can't remember the way the split was going to work.

@enavarro51
Copy link
Contributor Author

Parts of this were replaced by #10842, but #10869 is a new PR for displaying expressions. The rest of this one will have to be done in new future PRs.

@jakelishman jakelishman added the mod: visualization qiskit.visualization label Oct 3, 2023
@jakelishman jakelishman self-assigned this Oct 10, 2023
@jakelishman
Copy link
Member

Edwin: this appears to have quite a few merge conflicts at the moment - do you know what the status of this PR is now?

@enavarro51
Copy link
Contributor Author

I'll close it. Half of it is done in #10842 and the rest should be done in new PRs.

@enavarro51 enavarro51 closed this Oct 17, 2023
@jakelishman
Copy link
Member

Ah ok, that definitely makes things easier thanks!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants