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 display of registerless bits in all drawers and change display of one-bit registers #6942

Merged
merged 36 commits into from
Sep 13, 2021

Conversation

enavarro51
Copy link
Contributor

Summary

Fixes #6732
Fixes #5784

Details and comments

This PR fixes an issue where displaying circuits containing registerless bits would cause a failure of the 'mpl' and 'latex' circuit drawers. It also changes the display format in the 'text' drawer.

In addition, one-bit registers are now displayed in all drawers without a subscript 0. This standardizes the text formatting for 'latex' and 'mpl' drawers. All register names are now in italics, whether one-bit or more. Only the cregbundled registers are displayed in normal formatting.

In approaching this change, it became clear that the code for choosing a register name/number format was significantly different in the 3 drawers. Prior to this there were 4 if clauses which were done in 3 different orders, and this PR added 2 additional if clauses. It seemed prudent to standardize this selection process across the 3 drawers.

There is now a new function in utils.py called get_bit_label which returns the text that will be displayed for the registers and bits for all 3 drawers. This greatly simplified this code section in the 3 drawers and will make maintenance of register and bit names easier in the future.

There were quite a few reference image changes, but were mostly minor font or spacing changes in bit labels. Reviewers should still check these out for possible unacceptable changes.

qrx = QuantumRegister(2, "qrx")
qry = QuantumRegister(1, "qry")
crx = ClassicalRegister(2, "crx")
circuit = QuantumCircuit(qrx, [Qubit(), Qubit()], qry, [Clbit(), Clbit()], crx)
circuit.draw(cregbundle=True)

image

image

       
qrx_0: 
       
qrx_1: 
       
    2: 
       
    3: 
       
  qry: 
       
    0: 
       
    1: 
       
crx: 2/

@enavarro51 enavarro51 requested a review from a team as a code owner August 25, 2021 14:18
Copy link
Contributor

@javabster javabster left a comment

Choose a reason for hiding this comment

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

This is looking good, just a few minor comments. Moving generic code out into the utils file makes the drawer files much more readable, would love to see more enhancements like this in future!

@@ -146,6 +146,61 @@ def get_param_str(op, drawer, ndigits=3):
return param_str


def get_bit_label(drawer, register, index, qubit=True, layout=None, cregbundle=True):
"""Get the bit labels to display to the left of the wires."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add to the docstring what each of the arguments are please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8a7c24a.

@@ -0,0 +1,11 @@
---
fixes:
- |
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a v minor comment but I think it would be better to split this into 2 bullet points (one for each issue that was fixed), see code snippet example here in the contributing guidelines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8a7c24a.

@javabster javabster added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Sep 6, 2021
bit_label = f"{register.name}"
return bit_label

if register.size > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

This big nested if statement is kind of ugly but I understand it's partly because of messiness from before. At some point I think this should be refactored, but perhaps that is beyond the scope of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't help myself. Refactored in 8a7c24a. Let me know what you think.

Copy link
Contributor

@javabster javabster left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@kdk kdk added this to the 0.19 milestone Sep 13, 2021
@kdk kdk added the automerge label Sep 13, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drawing error in circuits with isolated qubits Inconsistency when drawing singleton registers
3 participants