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

Stop forcing mathtext mode for all strings #4669

Merged
merged 23 commits into from
Jul 28, 2020

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Jul 8, 2020

Summary

In #4616 the gate title and labels were changed to always use
matplotlib's mathtext mode. [1] This was a breaking change and will result
in users having weirdly formatted names and labels. For example places in
qiskit itself set gate names like 'Controlled-Evolution^1_dg' which will
not get rendered as expected in mathtext mode. The documented behavior for
labels and gate names before was to rely on matplotlib's rendering
behavior to expliclictly add '$' to the label string for sections that
should be rendered in mathtext mode. For example, this behavior was ported
to the latex circuit drawer in #3224 so that users could take advantage
of the same flexibility in how text was rendered by the drawer. This
commit removes the forced addition of '$' around all the label strings
to go back to the expected behavior.

Details and comments

Fixes #4667

[1] https://matplotlib.org/3.2.2/tutorials/text/mathtext.html

In Qiskit#4616 the gate title and labels were changed to always use
matplotlib's mathtext mode. [1] This was a breaking change and will result
in users having weirdly formatted names and labels. For example places in
qiskit itself set gate names like 'Controlled-Evolution^1_dg' which will
not get rendered as expected in mathtext mode. The documented behavior for
labels and gate names before was to rely on matplotlib's rendering
behavior to expliclictly add '$' to the label string for sections that
should be rendered in mathtext mode. For example, this behavior was ported
to the latex circuit drawer in Qiskit#3224 so that users could take advantage
of the same flexibility in how text was rendered by the drawer. This
commit removes the forced addition of '$' around all the label strings
to go back to the expected behavior.

Fixes Qiskit#4667

[1] https://matplotlib.org/3.2.2/tutorials/text/usetex.html
@mtreinish mtreinish requested review from maddy-tod, nonhermitian and a team as code owners July 8, 2020 14:52
@mtreinish mtreinish added this to the 0.15 milestone Jul 8, 2020
Previously the manual text width detection had a hard coded subset of
latex special characters to try and convert a latex string to a text
string. This however is quite error prone because it's missing a large
portion of valid latex commands. For example, in the previous commit
when we added \\mathrm to stop slanting text in the standard gates was
missed by this. Instead of maintaing a manual list of commands this
commit just switches to use pylatexenc, which is already a visualization
requirement for the latex drawers, to do the conversion from latex to
unicode. This is much less error prone and will give a true width of the
strings even for user supplied gate names which can use any latex
commands they want.
@Cryoris
Copy link
Contributor

Cryoris commented Jul 15, 2020

Could we change the subscripts to be uppercase and also non-slanted? Looks a bit weird now that in Rx, the x is cursive. Also if we use uppercase it'll be the same as the new IQX (which will hopefully be out soon).

@mtreinish
Copy link
Member Author

Could we change the subscripts to be uppercase and also non-slanted? Looks a bit weird now that in Rx, the x is cursive. Also if we use uppercase it'll be the same as the new IQX (which will hopefully be out soon).

Done in: ea4735e

'rzx': '$\\mathrm{R}_{\\mathrm{ZX}}$',
'rzz': '$\\mathrm{R}_{\\mathrm{ZZ}}$',
'reset': '$\\left|0\\right\\rangle$',
'initialize': '$|psi>$'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'initialize': '$|psi>$'
'initialize': '$|\\psi\\rangle$'

Copy link
Contributor

@enavarro51 enavarro51 Jul 19, 2020

Choose a reason for hiding this comment

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

I don't think this \\psi works with the default font. I get a 'Glyph missing from current font' message. If there's interest in replacing the psi with the Greek symbol, it can be done in unicode in python.
I believe this will work,

'initialize': '$|\U0001d713\\rangle$'

Here's the output
image

Copy link
Member Author

Choose a reason for hiding this comment

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

$\\psi\\rangle$ works fine on my system and I haven't changed the default fonts in mpl.

Screenshot_2020-07-20_07-35-25

It is probably dependent on your system fonts, and is not a matplotlib issue.

@Cryoris
Copy link
Contributor

Cryoris commented Jul 16, 2020

It seems the spacing in the multi-Pauli rotations is off:
see here
this is before:
here for what it was before

@enavarro51
Copy link
Contributor

enavarro51 commented Jul 18, 2020

@mtreinish @Cryoris The latex_to_text method leaves underscores in the output. That's why the multi-rotation Pauli's are wider above. The text width calc thinks there are 4 chars instead of 3. This code at line 268 will take care of it.

            nl_text = LatexNodes2Text().latex_to_text(text)
            if text[0] == '$':
                nl_text = nl_text.replace('_', '')

            f = 0 if fontsize == self._style.fs else 1
            sum_text = 0.0
            for c in nl_text:

It produces,
image
There is also an issue with the cu1 and zz gates with params appearing too wide and also negative params are coming out too wide. I'll take a look at these and leave a fix here sometime this weekend.

@enavarro51
Copy link
Contributor

On the cu1/zz issue dropping the '$$' from line 760, gate_width = (self._get_text_width(tname + ' ()',
seemed to take care of it.

This comit fixes a few edge case with mathmode text spacing. The first
is relying on pylatexenc to convert latex to text leaves '_' in for
subscripts in math mode strings. This fixes this by finding manually
removing those from the output string used for finding label width. The
other 2 fixes are from review comments about spacing for cui1/rzz and a
minus sign width.
@enavarro51
Copy link
Contributor

@mtreinish Don't know if you want to do this here or put in a separate issue. I was going through the textbook and found this,

qc = QuantumCircuit(1) # Create a quantum circuit with one qubit
initial_state = [0,1]   # Define initial_state as |1>
qc.initialize(initial_state, 0) # Apply initialisation operation to the 0th qubit
qc.draw()              # Let's view our circuit

image
There's a pretty simple fix. Around line 885 inside the 'initialize' section replace

                    self._multiqubit_gate(q_xy, fc=fc, ec=ec, gt=gt, sc=sc,
                                          text=gate_text, subtext=vec)

with

                    if len(q_xy) == 1:
                        self._gate(q_xy[0], fc=fc, ec=ec, gt=gt, sc=sc,
                                   text=gate_text, subtext=vec)
                    else:
                        self._multiqubit_gate(q_xy, fc=fc, ec=ec, gt=gt, sc=sc,
                                              text=gate_text, subtext=vec)

image

@1ucian0
Copy link
Member

1ucian0 commented Jul 21, 2020

Until the bot gets back on its feet. Here is the link for the tests!

https://mybinder.org/v2/gh/mtreinish/qiskit-core/fix-mathtext-mode-mpl-drawer?urlpath=apps/test/ipynb/mpl_tester.ipynb

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM, I couldn't find any corner cases anymore!

@mergify mergify bot merged commit b84590b into Qiskit:master Jul 28, 2020
@mtreinish mtreinish deleted the fix-mathtext-mode-mpl-drawer branch July 28, 2020 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matplotlib circuit drawer is unconditionally using latex mode for gate names
4 participants