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

Clean up text drawer compression issues #8588

Merged
merged 30 commits into from
Sep 2, 2022

Conversation

enavarro51
Copy link
Contributor

Summary

Fixes several problems with vertical_compression in the text circuit drawer.

Details and comments

There is an option for the text circuit drawer called vertical_compression that allows a user to choose how much vertical spacing there is between the horizontal lines. If 'high is chosen, then only 1 line will display between a pair of wires and in certain cases it's possible characters will overwrite other characters or drawing elements. If low is chosen, 2 lines will be used between every pair of lines and no overwriting will occur.

If medium, which is the default, is chosen, the compression starts out the same as high, but the drawer then tries to detect areas where overwriting would occur and expands the vertical to 2 lines where that would happen. The detection of these areas was not working consistently.

low compression was also causing connections between a gate and a control to be broken. Finally there were areas where connections for conditional gates were breaking. These drawings show 2 of the problems followed by a corrected drawing.

qr_0: ─░─────────────
       ░       ┌───┐ 
qr_1: ─░───■───┤ X ├─
       ░   │   Bar─1 
qr_2: ─░───┼─────░───
       ░ ┌─┴─┐   ░   
qr_3: ───┤ X ├───░───
         └───┘   ░   

       a             
qr_0: ─░─────────────
       ░       ┌───┐ 
qr_1: ─░───■───┤ X ├─
       ░   │   └───┘ 
       ░   │   Bar 1 
qr_2: ─░───┼─────░───
       ░ ┌─┴─┐   ░   
qr_3: ───┤ X ├───░───
         └───┘   ░   

qr_0: ────────────
            ┌───┐ 
qr_1: ──■───┤ X ├─
        │   └───┘ 
            Bar 1 
qr_2: ────────░───
      ┌─┴─┐   ░   
qr_3: ┤ X ├───░───
      └───┘   ░   

qr_0: ────────────
            ┌───┐ 
qr_1: ──■───┤ X ├─
        │   └───┘ 
        │   Bar 1 
qr_2: ──┼─────░───
      ┌─┴─┐   ░   
qr_3: ┤ X ├───░───
      └───┘   ░   

@qiskit-bot
Copy link
Collaborator

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:

@coveralls
Copy link

coveralls commented Aug 20, 2022

Pull Request Test Coverage Report for Build 2982196345

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 84.221%

Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/squ.py 2 79.78%
Totals Coverage Status
Change from base Build 2982049604: 0.002%
Covered Lines: 57176
Relevant Lines: 67888

💛 - Coveralls

@jakelishman jakelishman self-assigned this Aug 20, 2022
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! One minor comment about the tests, but I think the new behaviour is probably a pretty reasonable default.

Comment on lines 1691 to 1698
" ┌──────┐",
"q_0: |0>┤ my h ├",
" └──┬───┘",
"q_1: |0>───────",
" my ch ",
" ",
" c: 0 ═══════",
" 0x1 ",
" ┌──────┐ ",
"q_0: |0>──┤ my h ├",
" └──┬───┘ ",
"q_1: |0>─────■─────",
" my ctrl-h ",
" ",
" c: 0 ═════■═════",
" 0x1 ",
Copy link
Member

Choose a reason for hiding this comment

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

Can we revert this test case? The behaviour that appears with this PR and this test is probably something we want to make a positive decision on, and potentially test both it and your new form (with the space not aligned).

For clarity: with this PR, the original test produces

        ┌──────┐
q_0: |0>┤ my h ├
        └──┬───┘
q_1: |0>───■────
         my║ch
   c: 0 ═══■════
          0x1

instead. I feel like that (^) behaviour is probably actually suitable for vertical_compression=medium - it compresses vertically when there's a "space" so nothing visible gets erased, whereas "high" always compresses and "low" never does.

Perhaps leave the test as-is, and add your new one as a separate case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, I was hoping nobody would notice that. It works visually either way and it's probably a pretty rare case. I'll add the old test back with the new one.

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.

Looks good, thanks! Would you mind adding a short bugfix release note? Sorry - I missed that it wasn't there first time round apparently.

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.

Looks great, thanks for the changes!

@jakelishman jakelishman added Changelog: Bugfix Include in the "Fixed" section of the changelog automerge mod: visualization qiskit.visualization labels Sep 2, 2022
@mergify mergify bot merged commit aca01eb into Qiskit:main Sep 2, 2022
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