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

Classical condition boxes are now drawn as bullets in text drawer #6370

Merged
merged 25 commits into from
Jul 2, 2021

Conversation

TharrmashasthaPV
Copy link
Contributor

@TharrmashasthaPV TharrmashasthaPV commented May 6, 2021

Summary

Fixes #6290 .

Details and comments

In this PR, the classical condition boxes are replaced with open and close bullets when cregbundle=False. This ensures that the text drawer drawings of classical conditioned gates are consistent with that of the latex and mpl drawers.

For instance, the example circuit given in #6290 is now drawn as

ex_1

Update:
As it was suggested in the comment, the conditional value is also drawn along with the circuit. It looks like the following

     ┌───┐
q_0: ┤ H ├
     └─╥─┘
q_1: ──╫──
       ║  
c_0: ══o══
       ║  
c_1: ══■══
       =2 

Also the existing tests have been modified and new tests have been added.

@1ucian0
Copy link
Member

1ucian0 commented May 7, 2021

This is great! Can we still keep the hex representation of the condition?

     ┌───┐
q_0: ┤ H ├
     └─╥─┘
q_1: ──╫──
       ║  
c_0: ──o──
       ║  
c_1: ──■──
      0x2

Like MPL:
image

@TharrmashasthaPV
Copy link
Contributor Author

TharrmashasthaPV commented May 10, 2021

This is great! Can we still keep the hex representation of the condition?

     ┌───┐
q_0: ┤ H ├
     └─╥─┘
q_1: ──╫──
       ║  
c_0: ──o──
       ║  
c_1: ──■──
      0x2

Like MPL:
image

Hey @1ucian0 . I was actually unable to give the condition value as '0x2' as that would require a significant amount of code change. However, in the new circuit diagram the condition values are given as '=2'. I suppose that is fine.

@TharrmashasthaPV TharrmashasthaPV changed the title [WIP] Classical condition boxes are now drawn as bullets in text drawer Classical condition boxes are now drawn as bullets in text drawer May 10, 2021
@TharrmashasthaPV TharrmashasthaPV marked this pull request as ready for review May 10, 2021 00:01
@enavarro51
Copy link
Contributor

@TharrmashasthaPV #5938 fixes a problem with condition bits not reordering when reverse_bits is set. This was not addressed for the text drawer, because it wasn't showing the condition bits. Now that your PR fixes that, the reverse_bits issue should be addressed, either here or in a new issue. The fix is fairly simple

  • In circuit_visualization.py, pass reverse_bits to the text drawer.
  • In the text drawer __init__ add self._reverse_bits = reverse_bits
  • In set_cond_bullets, your current line 1474 could change to
vlist = list(val) if self._reverse_bits else list(val[::-1])

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Dont forget the reno!

@TharrmashasthaPV
Copy link
Contributor Author

@TharrmashasthaPV #5938 fixes a problem with condition bits not reordering when reverse_bits is set. This was not addressed for the text drawer, because it wasn't showing the condition bits. Now that your PR fixes that, the reverse_bits issue should be addressed, either here or in a new issue. The fix is fairly simple

  • In circuit_visualization.py, pass reverse_bits to the text drawer.
  • In the text drawer __init__ add self._reverse_bits = reverse_bits
  • In set_cond_bullets, your current line 1474 could change to
vlist = list(val) if self._reverse_bits else list(val[::-1])

Sorry @enavarro51 . I missed your comment somehow. Sure, I will fix that. Just to keep things a bit simple, I will push that as a different PR. Thanks a lot for pointing this out!!!

@TharrmashasthaPV TharrmashasthaPV requested a review from 1ucian0 May 26, 2021 12:29
@1ucian0
Copy link
Member

1ucian0 commented May 28, 2021

Let me know when you add the @enavarro51 fix . Once done, I think it will be ready to go.

@TharrmashasthaPV
Copy link
Contributor Author

TharrmashasthaPV commented May 31, 2021

Hi @1ucian0 . I have added the fix for the reverse_bits as suggested by @enavarro51 . I have also added two tests. I think it is now ready to go.

@javabster javabster added the Changelog: None Do not include in changelog label Jun 24, 2021
1ucian0
1ucian0 previously approved these changes Jun 24, 2021
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Thanks Tharrma!

@itoko itoko removed the automerge label Jul 1, 2021
@TharrmashasthaPV
Copy link
Contributor Author

I had to push a new commit since there were some conflicts with main. I have resolved the conflicts. The changes were minor. I guess this PR needs a re-review. Sorry for the inconvenience.

@TharrmashasthaPV TharrmashasthaPV requested a review from 1ucian0 July 1, 2021 12:19
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 LGTM 🚀 @1ucian0 thoughts?

@1ucian0 1ucian0 added Changelog: API Change Include in the "Changed" section of the changelog and removed Changelog: None Do not include in changelog labels Jul 2, 2021
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mergify mergify bot merged commit 15edb0b into Qiskit:main Jul 2, 2021
TharrmashasthaPV added a commit to TharrmashasthaPV/qiskit-terra that referenced this pull request Jul 13, 2021
mergify bot added a commit that referenced this pull request Jul 14, 2021
…onditioning (#6261)

* Cbit conditional

* lint fixed

* q

* added_tests

* Added tests

* lint fix

* lint-fix

* added extra test

* unwanted comment removed

* refactored for bit.register deprication

* text drawer support for single bit cond

* resolved conflicts

* Added reno

* Update utils.py

* Removing redundant structures

* lint fix

* blacking and linting

* Black fix

* Corrected tests compression and refactored for #6370

Co-authored-by: Thomas Alexander <[email protected]>
Co-authored-by: Luciano Bello <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistency in drawing classical control in text drawer when cregbundle=False
5 participants