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

MPL drawer extended for drawing circuits with single classical bit conditioning #6259

Merged
merged 51 commits into from
Sep 15, 2021

Conversation

TharrmashasthaPV
Copy link
Contributor

@TharrmashasthaPV TharrmashasthaPV commented Apr 20, 2021

Summary

Fixes part of #6475 . This PR extends the ability of the current mpl drawer to draw circuits that contain gates or instructions that are conditioned on a single classical bit.

Details and comments

This PR follows #6018 . #6018 breaks the mpl drawer when drawing circuits that contain instructions with single bit conditioning. This is an attempt at fixing this error. Two tests have been added. Examples of the mpl drawing are as below.

With cregbundle=True:
circ-1

With cregbundle=False:
circ-2

@TharrmashasthaPV TharrmashasthaPV changed the title [WIP] MPL drawer extended for drawing circuits with single classical bit conditioning MPL drawer extended for drawing circuits with single classical bit conditioning May 28, 2021
@TharrmashasthaPV TharrmashasthaPV marked this pull request as ready for review May 28, 2021 04:53
if cond_is_bit and self._cregbundle:
cond_reg = self._bit_locations[op.condition[0]]["register"]
ctrl_bit = self._bit_locations[op.condition[0]]["index"]
label = "%s_%s=%s" % (cond_reg.name, ctrl_bit, hex(val))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to do this in math notation "$%s_%s$=%s" so that is comes out a subscript instead of an underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subscript is a good idea. But I just feel that the subscript would be very small. It would look something like this

image

If this is still fine, I think subscript is more preferable since it also saves some space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed it is small. Since it should be zoomable on most devices, I'm ok with it. Probably one for final reviewer to decide.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think better to leave it with the underscore so it is more accessible for users with limited eyesight, devices without zoom capabilities, screenshots etc.

Comment on lines 869 to 872
for index, cbit in enumerate(self._clbit):
if self._bit_locations[cbit]["register"] == node.op.condition[0]:
mask |= 1 << index
if cond_is_bit:
for index, cbit in enumerate(self._clbit):
if cbit == node.op.condition[0]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have an extra for loop at line 869.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Thanks for pointing to out. Fixed it in 1d9c58f .

enavarro51
enavarro51 previously approved these changes Sep 3, 2021
Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes.

@1ucian0 1ucian0 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Sep 6, 2021
@javabster
Copy link
Contributor

hey @TharrmashasthaPV when I run the binder tests at https://mybinder.org/v2/gh/TharrmashasthaPV/qiskit-terra/issue1160mpl?urlpath=apps/test/ipynb/mpl_tester.ipynb i see a few failures that should be fixed before this gets merged in 😄

@TharrmashasthaPV
Copy link
Contributor Author

hey @TharrmashasthaPV when I run the binder tests at https://mybinder.org/v2/gh/TharrmashasthaPV/qiskit-terra/issue1160mpl?urlpath=apps/test/ipynb/mpl_tester.ipynb i see a few failures that should be fixed before this gets merged in 😄

Oh. Sorry for that. I have replaced the incorrect reference image with the right ones in 7dc2d59. I also made a small change to have hex under the control bit even for single bit conditions when cregbundle=False (This was not there in the previous commit ) like below:

image

and brought back the underscore.

image

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 and snapshots now pass 👏

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!

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.

6 participants