Tweak colors by galeinston · Pull Request #5063 · Qiskit/qiskit

galeinston · 2020-09-14T04:33:45Z

Summary

Details and comments

Modified background colors of operational gates and their font text colors to match with IQX colors as suggested by @1ucian0 in the original issue.

enavarro51

@1ucian0 Need clarification on some colors.
Gate Previous New
cx Blue DarkBlue
ccx Purple DarkBlue
mcx Purple ?
swap Blue DarkBlue
cswap Purple DarkBlue
ccswap Purple ?
dcx Blue ?
iswap Blue ?
What about u and p?
I assume all controlled versions of t, s, sdg, and tdg will be DarkRed?
I also assume all other gates - custom to_gates, isometry, diagonal, gms, etc., etc. will be DarkRed?
Thanks.

enavarro51 · 2020-09-14T14:11:55Z

qiskit/visualization/matplotlib.py

+                # ccx gates
+                elif isinstance(op.op, ControlledGate) and op.name == 'ccx':
+                    num_ctrl_qubits = op.op.num_ctrl_qubits
+                    self._set_ctrl_bits(op.op.ctrl_state, num_ctrl_qubits,
+                                        q_xy, ec=ec, tc=tc, text=ctrl_text, qargs=op.qargs)
+                    self._x_tgt_qubit(q_xy[num_ctrl_qubits+1], ec=ec,
+                                      ac=self._style.dispcol['target'])
+                    self._line(qreg_b, qreg_t, lc=lc)
+


I don't believe this elif is necessary. The ccx is covered by the base_name == 'x' elif above it.

If I don't specifically add the elif here, the color would be in dark red instead of dark blue (please check the reference in #5032). The present version doesn't handle this scenario either, i.e. >=2 controlled qubit, the color is different from colors defined for swap and cx

I see you added ccx to dispcol in qcstyle. This should take care of the color. I haven't run your code yet, but when I add it in to qcstyle with a different color using the master version, the color changes in the display output.

enavarro51 · 2020-09-14T14:14:53Z

qiskit/visualization/matplotlib.py

-                elif op.name != 'swap' and base_name == 'swap':
+                elif op.name == 'cswap':


This will not cover swap gates with more than one control, like ccswap, so should go back to original.

Yeah. I also figured this problem. The current version also doesn't cover for cases with more than 1 controlling qubit. Since the original problem asked for colors change only, I modified the code to match it up with IQX reference. Should we create a different issue for it?

I understand why you did it, but we have to be careful we don't lose functionality. Adding a separate elif would be better. I left a comment on the original issue. I feel like we're working with an incomplete spec.

The specifications are complete, see the comment below: #5063 (comment)

1ucian0 · 2020-09-15T13:36:49Z

@1ucian0 Need clarification on some colors.

I have no opinion here. @nonhermitian ?

galeinston · 2020-09-15T14:47:58Z

@1ucian0 Since the color outline is still under discussion. Should I run the binder as in #4544 or put it on hold?

Another question: there are 2 active PRs right now to solve #5032. Should we keep updating our code simultaneously like this?

Cryoris · 2020-09-16T07:58:37Z

@enavarro51 @1ucian0 here's an overview of the colors: https://quantum-computing.ibm.com/docs/iqx/operations-glossary.

Classical gates are dark blue, this includes also MCX and DCX, since they are also a classical operation.
Phase gates (this means diagonal matrices) are light blue, this includes also P, T and S as well as their inverses. Non-unitary operations, such as c_if, barrier, measure and reset are grey. All other quantum gates are dark red, i.e. custom to_gate etc.

So here's the overview again, including the gates @enavarro51 mentioned above:

Classical: Dark blue
I, X, CX, CCX, C3X, C4X, any MCX, DCX, Swap, CSwap, and controlled versions of these.

Phase gates: Cyan
Z, S, Sdg, T, Tdg, P, U1

Hadamard: Red
H

Non-unitary: Grey
Reset, c_if, Barrier, Measure

Other: Dark red
Every gate not mentioned above, also arbitrary custom gates, U, U3, GMS, SX, etc.

In principle, controlled versions of the phase gates should also be cyan as controlling a diagonal gates is still diagonal. But this might be a little more complicated to do and we can stick to just making the single qubit ones cyan for now and the controlled versions have the default red.

Cryoris · 2020-09-16T08:02:06Z

Another question: there are 2 active PRs right now to solve #5032. Should we keep updating our code simultaneously like this?

I don't think this makes a lot of sense, it'll just duplicate the work and we'll be only able to merge one 🤔

Cryoris · 2020-09-16T08:04:01Z

qiskit/visualization/qcstyle.py

+        basis_color = '#FA74A6'               # Red
+        clifford_color = '#6FA4FF'            # Blue
+        iden_color = '#05BAB6'                # Green
+
+        hadamard_color = '#DC143C'            # Red
+        classical_gate_color = '#000080'      # Dark Blue
+        phase_gate_color = '#1E90FF'          # Light Blue
+        non_unitary_gate_color = '#808080'    # Grey
+        other_color = '#99004C'               # Dark Red


These are the official color codes, could you update these?

classical: #002D9C phase: #33B1FF hadamard: #FA4D56 quantum: #9F1853 non-unitary: #A8A8A8

@Cryoris absolutely

Cryoris · 2020-09-16T08:33:11Z

qiskit/visualization/matplotlib.py

+            gt = self._style.lc
+        else:
+            gt = self._style.gt
+        if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'reset', 'meas', 'measure'}:
            gt = self._style.not_gate_lc
+            self._style.sc = self._style.not_gate_lc
        else:
            gt = self._style.gt
+            self._style.sc = self._style.lc


The text colors should not be hardcoded in matplotlib.py but should be a dictionary in the Style class like the gate color. Otherwise we won't be able to use different style sheets that use different combinations of gate and text color.

Understood. I'll fix it

enavarro51 · 2020-09-16T13:52:37Z

@Cryoris Thanks for the great info. This clarifies a lot.

enavarro51 · 2020-09-16T14:31:13Z

I have a proposal that will get us well on the way to a true user-defined colormap/style sheet and can be done as an extension of this PR.

There already exists a rudimentary style sheet idea in the mpl drawer, but it is incomplete and buggy. In fact, I'm relatively sure there is nobody using the style capability for 'displaycolor' since it would only work if they knew the source and knew one specific way to set it up. It would be fairly easy to make the 'displaycolor' option available to users.

In addition there is a user-configurable selection in the settings.conf file for 'circuit_mpl_style', but the only option that works in the mpl code is 'default', which is what you would get anyway. So I think we should add 'original' - for current colors, 'iqx' - for the new iqx color map, and 'bw' for black & white. Then we just need 3 style classes in qcstyle to do it all.

Also there is an issue with font color, since there are a lot of border cases where black or white could both work, so I'd like to make the dispcol value a (color, font_color) tuple which would make the qcstyle code much easier to maintain. Unlike the old version of mpl, which had 35 references to dispcol, the new version has 3 (pat myself on the back :)), so updating this in mpl would be easy as well, and can be made backward compatible, in case anyone was using the 'displaycolor' option.

This would be phase 1 on the path to full user colormap definition. It would,

Provide 3 color styles, original, iqx, and bw, selectable by the user
Create a structure for future imports of user-styles
Allow the user to configure specific gate colors through the style 'displaycolor' param to draw

Phase 2 would create a location for colormaps, maybe .qiskit, and let users build whatever they wanted and import somehow into mpl, either as an import or a file read.

I'm happy to work with @galeinston on this. It really should be fairly straightforward.

galeinston · 2020-09-16T14:55:32Z

I like the idea of 3 color styles and how @enavarro51 planned out to make it work. It's be my pleasure to work on the proposed extension.

mtreinish · 2020-09-16T15:15:58Z

I have a proposal that will get us well on the way to a true user-defined colormap/style sheet and can be done as an extension of this PR.

There already exists a rudimentary style sheet idea in the mpl drawer, but it is incomplete and buggy. In fact, I'm relatively sure there is nobody using the style capability for 'displaycolor' since it would only work if they knew the source and knew one specific way to set it up. It would be fairly easy to make the 'displaycolor' option available to users.

FWIW, we do try to document the style sheet style parameter which does let users define a full style dict/ json for the drawer to use: https://qiskit.org/documentation/stubs/qiskit.visualization.circuit_drawer.html#qiskit.visualization.circuit_drawer But I agree it could definitely use some love.

In addition there is a user-configurable selection in the settings.conf file for 'circuit_mpl_style', but the only option that works in the mpl code is 'default', which is what you would get anyway. So I think we should add 'original' - for current colors, 'iqx' - for the new iqx color map, and 'bw' for black & white. Then we just need 3 style classes in qcstyle to do it all.

I agree with this, instead of changing the default this PR should be adding a 3rd iqx class to qcstyle.py. For the settings.conf we should just iqx to list of valid default styles in qiskit/user_config.py I think what you're calling original here should probably stay as default (just for backwards compat reasons. Besides the settings.conf file, default is the kwarg default value for the draw functions. If we do rename default to original we'll still have to correctly handle when default is set as the value in the settings.conf and kwarg.

Also there is an issue with font color, since there are a lot of border cases where black or white could both work, so I'd like to make the dispcol value a (color, font_color) tuple which would make the qcstyle code much easier to maintain. Unlike the old version of mpl, which had 35 references to dispcol, the new version has 3 (pat myself on the back :)), so updating this in mpl would be easy as well, and can be made backward compatible, in case anyone was using the 'displaycolor' option.

This would be phase 1 on the path to full user colormap definition. It would,
* Provide 3 color styles, original, iqx, and bw, selectable by the user

* Create a structure for future imports of user-styles

* Allow the user to configure specific gate colors through the style 'displaycolor' param to draw

I think this last point might be out of scope for this PR. I'm assuming by this you're talking about doing something like:

circuit_drawer(qc, output='mpl', style={'displaycolor': {'h': '#ff0080'}})

and instead of the current behavior where only hadamard gates would have a color set, it changes the default color in the configured style sheet to use the fuschia for hadmard and everything remains the same. We should do this piece separately because there are backwards compat implications for changing this behavior. We can quickly add a new stylesheet to the drawer for the iqx theme (and fix the font color thing as you described) without this piece.

Phase 2 would create a location for colormaps, maybe .qiskit, and let users build whatever they wanted and import somehow into mpl, either as an import or a file read.

My thinking on this was to either do it one of 2 ways:

We add a mpl_stylesheet dir to ~/.qiskit (and allow it to be configured via an env var and entry in the settings.conf). Then we just locate all the .json files in that dir and use the filename as the style name.
We add a config section to the settings.conf for mpl style sheetsheet where we add entries for style name and path to json. For example:

[mpl_stylesheets]
custom1 = ~/custom_mpl_style.json
custom2 = ~/special_mpl_style.json

I'm leaning more towards the later, because it also gives us this flexibility for more easily distinguishing style sheets between the mpl circuit drawer, the pulse drawer (and possibly the mpl state visualizations too). The pulse visualizer has the same kind of style sheets as the circuit drawer and whatever we expose for the circuit drawer we're also going to want to work for the pulse visualizer too.

enavarro51 · 2020-09-16T15:53:52Z

I agree with this, instead of changing the default this PR should be adding a 3rd iqx class to qcstyle.py. For the settings.conf we should just iqx to list of valid default styles in qiskit/user_config.py I think what you're calling original here should probably stay as default (just for backwards compat reasons. Besides the settings.conf file, default is the kwarg default value for the draw functions. If we do rename default to original we'll still have to correctly handle when default is set as the value in the settings.conf and kwarg.

Using 'default' is fine.

I think this last point might be out of scope for this PR. I'm assuming by this you're talking about doing something like:
circuit_drawer(qc, output='mpl', style={'displaycolor': {'h': '#ff0080'}})
and instead of the current behavior where only hadamard gates would have a color set, it changes the default color in the configured style sheet to use the fuschia for hadmard and everything remains the same. We should do this piece separately because there are backwards compat implications for changing this behavior. We can quickly add a new stylesheet to the drawer for the iqx theme (and fix the font color thing as you described) without this piece.

There actually is not a backwards compat problem here. Someone can currently do what you show above, but if they put an 'x' gate in the circuit, the drawer would crash. The 'displaycolor' dict currently has to have an entry for every gate that's used. Not sure anybody really knows that.

What I'm proposing is doing the same thing, but not crashing. As you say, this would then change the 'h' color and leave all the rest the same. Don't think we need to worry about backwards compat if the previous behavior was a crash.

And making the (color, font_color) tuple can be backwards compat as well by checking if the value for 'h' is a string or tuple in the code. If a string, use the color and make the font_color either the default or what the user has defined in the style dict.
So the new way would now look like,
circuit_drawer(qc, output='mpl', style={'displaycolor': {'h': ('#ff0080', '#000000')}})
It's not real elegant for the user, but the capability is there and anything they were previously doing would still work.

All of this will require some doc changes, of course.

As to the phase 2, what you suggest looks fine and making it compatible with the pulse drawer would be great. Once this PR is done, we can create a new issue for phase 2.

enavarro51 · 2020-09-17T17:08:29Z

@galeinston Are you on qiskit Slack?

galeinston · 2020-09-17T18:03:52Z

Yup. I'm on Slack

galeinston · 2020-09-17T22:40:51Z

Following @1ucian0 suggestion in #4544,I made this binder to test the MPL. I believe this link is still valid even after @enavarro51 and I update the code based on #5063 (comment) and #5063 (comment)
👇

enavarro51 · 2020-09-17T23:31:02Z

Phase gates: Cyan
Z, S, Sdg, T, Tdg, P, U1

@Cryoris Just one more check. What about sx and sxdg. Are these phase gates? Thanks.

abhobe · 2020-09-18T03:09:44Z

@galeinston Make sure to update the refs in /test/ipynb/mpl/references to match your gate colors. Or else you will have failing binder tests.

abhobe

Here are some small changes.

abhobe · 2020-09-18T03:47:01Z

qiskit/visualization/matplotlib.py

+            gt = self._style.lc
+        else:
+            gt = self._style.gt
+        if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'r', 'reset', 'meas', 'measure'}:


Suggested change

if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'r', 'reset', 'meas', 'measure'}:

if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'reset', 'meas', 'measure'}:

I think r should be removed because it seems like its a quantum gate which needs a white font.

Binder Tests

Cryoris · 2020-09-18T07:53:50Z

@Cryoris Just one more check. What about sx and sxdg. Are these phase gates? Thanks.

@enavarro51 SX and SXdg should be red, their matrix representation is not diagonal. I updated the list above 👍

enavarro51 · 2020-09-20T21:48:45Z

Classical: Dark blue
I, X, CX, CCX, C3X, C4X, any MCX, DCX, Swap, CSwap, and controlled versions of these

@Cryoris Some (hopefully) last questions on colors.

c3x and c4x are no longer active. You can't enter them as circuit.c3x and if you create a 3-control x, it returns mcx as the name.
if you enter circuit.mcx it returns mcx_gray as the name. Should we add this to the color list?
DCX, for example, can have an unlimited number of controls, so c6dcx could be a gate name. What we're trying to do here is a simple mapping of gate names to colors, which is what we have always done. Is it ok if we stop the number of controls for Dark Blue at some reasonable number like 2 (for other than X). So cdcx and ccdcx would be DarkBlue and any higher # of controls would be DarkRed? Similar for Swap?

galeinston · 2020-09-21T02:29:41Z

@Cryoris I also have a question regarding the R-gate (as shown in the picture). I can't find it neither in Circuit Library document nor in IQX page, can you clarify if this gate has been deprecated/deleted?

enavarro51 · 2020-09-23T18:27:44Z

We've made the following changes in this PR.

First, all changes should be 100% backward compatible. We've only added functionality, not taken any away.
There are now 3 color styles available - default, iqx, and bw. They can be set in one of 2 ways.
- In the [Default] section of the settings.conf file in the ~/.qiskit dir, add circuit_mpl_style = '<style_name>'
- In the draw() command add param style={'name': '<style_name>'}
There needed to be some special mpl drawer code added to handle controlled classical gates, so iqx will not be as generic as other possible styles. The problem was with gate names like c5dcx, c6dcx, and onward.
The dispcol dict in the style dict now sets colors as (gate_color, text_color). This allows the use of light and dark gate colors with black or white text. Actually someone could configure the style dict to do any combo, such as dark blue text on a yellow gate. The previous dispcol format of gate_color only still works, and will in that case use the 'gatetextcolor' for text.
The style dict docs in quantumcircuit.py and circuit_visualization.py have been updated with the current dispcol, with explanation changes as well.
A binder test was added for the iqx colors.
A release note will be done once any replies to this comment have been addressed.

Phase 2 for the future,

This PR creates 3 separate style classes in qcstyle.py. We envision that in future, this can be reduced to 1 default class and the additional styles would be read in from json files. I don't believe users should be allowed to modify these 3 styles directly, but they can copy these and create new styles of their own. Which brings up where to put things in the future.
- I believe that default, iqx, and bw json style files should be part of qiskit, perhaps in the visualization directory.
- User json files should go in a fixed place, perhaps in a styles dir under ~/.qiskit.
The code for reading in the json files already exists, though there is only minimal error checking currently. Deciding on the locations for the files is the main issue.
We might want to look at any possible special cases, like the controlled classical gates in iqx and see if any of these can be addressed in a generic way. Right now all a user can do with color, is change the colors of the specific gate names in dispcol.

…it-terra into Tweak_Colors_IQX

1ucian0

Thanks!

Tweak colors to match new IQX colors

5fa5898

galeinston requested review from a team, maddy-tod and nonhermitian as code owners September 14, 2020 04:33

enavarro51 suggested changes Sep 14, 2020

View reviewed changes

abhobe mentioned this pull request Sep 14, 2020

Tweak Colors to Match IQX Light Theme #5065

Closed

Cryoris reviewed Sep 16, 2020

View reviewed changes

1ucian0 mentioned this pull request Sep 17, 2020

Change colors to match IQX White Theme #5066

Closed

Testing push commit

660bd00

Merge branch 'master' into Tweak_Colors_IQX

08fa8a9

abhobe suggested changes Sep 18, 2020

View reviewed changes

galeinston changed the title ~~Tweak colors~~ [WIP] Tweak colors Sep 19, 2020

Minor qcstyle changes

c994f70

1ucian0 marked this pull request as draft September 22, 2020 20:21

enavarro51 added 6 commits September 22, 2020 16:27

Allow str or tuple for displaycolor and update docs

befb689

Merge branch 'master' into Tweak_Colors_IQX

394e6fa

Doc string changes

2971c46

Further update docs

14456d8

Merge branch 'master' into Tweak_Colors_IQX

bcf09e6

Add name in style param to choose ipx or bw and add binder test

4221874

enavarro51 added 7 commits September 23, 2020 13:25

Add style to test and fix typos

fc7709d

Rework style name and add name to style docs

49a649f

Add reno

80fbd86

Minor name mods

2117b78

Merge branch 'master' into Tweak_Colors_IQX

70f7034

Fix indent

25bd9bb

Merge branch 'Tweak_Colors_IQX' of https://github.com/galeinston/qisk…

4fd55bf

…it-terra into Tweak_Colors_IQX

1ucian0 marked this pull request as ready for review September 24, 2020 21:01

1ucian0 changed the title ~~[WIP] Tweak colors~~ Tweak colors Sep 24, 2020

enavarro51 added 7 commits September 26, 2020 14:56

Merge branch 'master' into Tweak_Colors_IQX

25798af

Fix style selection

9e9e448

Merge branch 'master' into Tweak_Colors_IQX

84edcf5

Merge branch 'Tweak_Colors_IQX' of https://github.com/galeinston/qisk…

498bee4

…it-terra into Tweak_Colors_IQX

Fix lint and imports

5233a8e

Lint again

cb57699

Merge branch 'master' into Tweak_Colors_IQX

3fa5787

Qiskit deleted a comment from enavarro51 Sep 28, 2020

1ucian0 approved these changes Sep 28, 2020

View reviewed changes

1ucian0 merged commit cfbe3ff into Qiskit:master Sep 28, 2020

enavarro51 mentioned this pull request Oct 1, 2020

Make mpl colors and other styles fully configurable by users with a json load #5169

Closed

abhobe mentioned this pull request Oct 3, 2020

Q-sphere phase colors does not match IQX phase colors #5167

Closed

		elif op.name != 'swap' and base_name == 'swap':
		elif op.name == 'cswap':

	if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'r', 'reset', 'meas', 'measure'}:
	if op.name not in {'h', 't', 's', 'z', 'sdg', 'tdg', 'u1', 'reset', 'meas', 'measure'}:

Conversation

galeinston commented Sep 14, 2020

Summary

Details and comments

Uh oh!

enavarro51 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galeinston Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cryoris Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ucian0 commented Sep 15, 2020

Uh oh!

galeinston commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cryoris commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cryoris commented Sep 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enavarro51 commented Sep 16, 2020

Uh oh!

enavarro51 commented Sep 16, 2020

Uh oh!

galeinston commented Sep 16, 2020

Uh oh!

mtreinish commented Sep 16, 2020

Uh oh!

enavarro51 commented Sep 16, 2020

Uh oh!

enavarro51 commented Sep 17, 2020

Uh oh!

galeinston commented Sep 17, 2020

Uh oh!

galeinston commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enavarro51 commented Sep 17, 2020

Uh oh!

abhobe commented Sep 18, 2020

Uh oh!

abhobe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cryoris commented Sep 18, 2020

Uh oh!

enavarro51 commented Sep 20, 2020

Uh oh!

galeinston commented Sep 21, 2020

Uh oh!

enavarro51 commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enavarro51 left a comment •

edited

Loading

galeinston Sep 14, 2020 •

edited

Loading

Cryoris Sep 16, 2020 •

edited

Loading

galeinston commented Sep 15, 2020 •

edited

Loading

Cryoris commented Sep 16, 2020 •

edited

Loading

galeinston commented Sep 17, 2020 •

edited

Loading

enavarro51 commented Sep 23, 2020 •

edited

Loading