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 register names with multiple underscores in mpl and latex drawers #7459

Merged
merged 19 commits into from
Jan 13, 2022

Conversation

enavarro51
Copy link
Contributor

Summary

Fixes #7455

Details and comments

An attempt was made to deal with register names containing multiple underscores in #3827, but it did not work in all instances. This PR fixes that. The same problem would also crash the latex drawer and this has been fixed as well.

This PR now assumes any register names coming into the drawers will conform to the OpenQASM 2.0 spec. In the current main, this is not guaranteed. See #7458.

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.

This seems to be part of a more general problem, that register names may contain special characters that can't be safely inserted into the formatters for the drawers. We'd have similar problems if a register contained ^, and everything would break horrendously if { was present without a }.

I think it might be a better strategy to keep a mapping of register: escaped_name somewhere, and then have each drawer keep its own function for safely escaping names. That'll be easier to extend in the future, and I think it ought to simplify some of the _get_text_width handling; instead of counting all the underscores/carets/whatever and then trying to correct for it later, we should possibly just scan through a math-mode match, and only count unescaped special characters. Then it doesn't matter how the escaped characters got in, it'll still work correctly.

OpenQASM 3 will add whole chunks of Unicode characters into the valid identifiers, so restricting to /[a-zA-Z_][a-zA-Z0-9_]*/ is going to be too restrictive, and it's probably a good idea for us to avoid restricting unrelated parts of Terra's behaviour on the basis of the OpenQASM 3 spec (which isn't part of Terra).

@enavarro51
Copy link
Contributor Author

@jakelishman So this takes care of the problem with #7455 and fixes another problem with the text width if _s were used in a name. Multiple underscores would be removed in width calc which would cause the width to appear too small. Result was the name would run off the left side. The solution was to only remove 1 _ if register.size > 1, otherwise don't remove any. For gate names and labels, nothing is changed.

It also solves the problem for the latex drawer which would fail on multiple _s.

As to the bigger picture, you're right, names like abc^{def would be a mess all around. And problem is if the error gets down to the matplotlib or LaTex level, the error messages are generally not very helpful.

@enavarro51
Copy link
Contributor Author

This also adds tests that weren't included in #3827.

@coveralls
Copy link

coveralls commented Jan 5, 2022

Pull Request Test Coverage Report for Build 1693401113

  • 13 of 22 (59.09%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.003%) to 83.098%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/matplotlib.py 1 10 10.0%
Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 1692656903: -0.003%
Covered Lines: 51988
Relevant Lines: 62562

💛 - Coveralls

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 PR fixes the original issue but I do agree with @jakelishman that it would be great if special characters could be handled in a generic way and could also include support for OpenQASM3 characters. It would be nice to do this as part of this PR but understand it wasn't part of the original issue requirements so perhaps an intermediate solution for this PR could be to convert _fix_double_script to a more generic function (stored in utils.py) that for the moment just handles the underscores but could be expanded later when we want to address other special characters? Then @jakelishman could open a separate issue covering the cases he described?

@enavarro51
Copy link
Contributor Author

How about a fix_special_characters added to utils.py that takes a string and returns the string with whatever adjustments are required. We can add kwargs as needed for special handling of some strings, but for now just fix the mulit-underscores.

@enavarro51
Copy link
Contributor Author

Added fix_special_characters function to utils.py in ad5010a.

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.

Yeah I like this better with a more generic fix_special_characters function thanks! Although just looking through the drawers now, was thinking does it make more sense to call fix_special_characters from get_bit_label? Then we don't need to call it multiple times in the drawers

qiskit/visualization/utils.py Outdated Show resolved Hide resolved
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 for this - the new separate function looks like a neater way of handling things to me. Certainly it's easier for me to follow the code this way through. I just left a couple of minor comments (and Abby's comment), but this is certainly moving to merge.

The new tests look good - thanks for those.

qiskit/visualization/utils.py Outdated Show resolved Hide resolved
qiskit/visualization/matplotlib.py Outdated Show resolved Hide resolved
qiskit/visualization/matplotlib.py Show resolved Hide resolved
@enavarro51
Copy link
Contributor Author

@javabster I went ahead and did what you suggested on putting the special char call inside get_bit_label in 0a5356e. I discovered that all the latex creg labels were not wrapped in {} when cregbundle is on, unlike what happens with all the other bit labels. It works either way, but I thought it might cause a tricky bug if left like it was. This is why there are a bunch of updated latex refs.

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.

This looks good to me now, thanks for the changes, and for the stewardship of the drawers!

@jakelishman jakelishman added automerge Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable labels Jan 13, 2022
@jakelishman jakelishman added this to the 0.19.2 milestone Jan 13, 2022
@mergify mergify bot merged commit a14e626 into Qiskit:main Jan 13, 2022
mergify bot pushed a commit that referenced this pull request Jan 13, 2022
…tex drawers (#7459)

* Fix multi-underscore display

* Fix register num subscript

* Fix spacing in register names

* Update ref images

* Refine text_width calc for registers

* Create fix_special_characters function

* Lint

* Move call to fix special chars into get_bit_label and update latex refs

* Fix mpl cregbundle

* Lint

* Fix underscore and spacing and lint

* Doc string

(cherry picked from commit a14e626)
mergify bot added a commit that referenced this pull request Jan 13, 2022
…tex drawers (#7459) (#7527)

* Fix multi-underscore display

* Fix register num subscript

* Fix spacing in register names

* Update ref images

* Refine text_width calc for registers

* Create fix_special_characters function

* Lint

* Move call to fix special chars into get_bit_label and update latex refs

* Fix mpl cregbundle

* Lint

* Fix underscore and spacing and lint

* Doc string

(cherry picked from commit a14e626)

Co-authored-by: Edwin Navarro <enavarro@comcast.net>
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 stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'_' makes matplotlib to render the following character as subscript
4 participants