fix hinton visualization bug #8447

wyqian1027 · 2022-08-04T08:48:37Z

Summary

Details and comments

I corrected
ax2.set_xlim([0, lx]) ax2.set_ylim([ly, 0])
and added
fig.tight_layout()

Tests are done on my computer to cover the change so it looks aligned:

And the spacing is fixed for a larger density matrix:

One thing is that now the diagonal of the density matrix runs from lower left to upper right, which is changed from the previous function, but it is just a style thing.

qiskit-bot · 2022-08-04T08:48:39Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@enavarro51

CLAassistant · 2022-08-04T08:48:42Z

All committers have signed the CLA.

CLAassistant · 2022-08-04T08:48:42Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

HuangJunye · 2022-08-04T14:20:45Z

Thanks for submitting the PR for fixing the issue you found. As this is your first time contributing to Qiskit Terra, please have a look at the general Qiskit contributing guideline (especially the "Pull Request Checklist" section) and the CONTRIBUTING.md in Terra.

If you are unsure about anything, please feel free to ping me. Thanks!

HuangJunye · 2022-08-04T14:22:14Z

Can you please write a release note and sign the Contributor License Agreement?

coveralls · 2022-08-04T14:30:29Z

Pull Request Test Coverage Report for Build 3277537504

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
117 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-0.2%) to 84.602%

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/quantum_initializer/squ.py	2	79.78%
src/sabre_swap/neighbor_table.rs	7	69.23%
src/sampled_exp_val.rs	14	63.93%
src/results/marginalization.rs	37	67.82%
src/optimize_1q_gates.rs	57	4.84%

Totals
Change from base Build 3276042896:	-0.2%
Covered Lines:	61823
Relevant Lines:	73075

💛 - Coveralls

wyqian1027 · 2022-08-04T16:19:45Z

Can you please write a release note and sign the Contributor License Agreement?

I think I have signed the Contributor License earlier today, but I am not sure about where to write a release note. Could you guide me through that? Thank you!

edits: Ok I see, I am trying to add the release note with reno now.
edits: Ok, I added release notes under releasenotes/notes/fix-hinton-bug-1141a297050f55bb.yaml

…erra into fix_hinton_bug

HuangJunye

Thanks for adding the release note. I left some comments for that.

Reading your original message on top, I realize the y-axis now is reverted to the normal direction (lower at the bottom, higher at the top). I am not sure about the original intention, but I believe there is a reason for the inverted direction. I will investigate that a bit.

HuangJunye · 2022-08-05T08:40:29Z

releasenotes/notes/fix-hinton-bug-1141a297050f55bb.yaml

+issues:
+  - |
+    Fix known bug in plot_state_hinton where axes are rendered improperly.


Suggested change

issues:

- |

Fix known bug in plot_state_hinton where axes are rendered improperly.

This part is not needed. For PR that fixes an issue, you only need to write fixes section, as far as I know we don't use issues section that much in Qiskit. Thanks for reading through the description of different sections of the release note though.

I know writing release notes are not easy. I struggle with it all the time :D For the future, if you are unsure about this, you can read other examples in the release notes folder: https://github.com/Qiskit/qiskit-terra/tree/main/releasenotes/notes

HuangJunye · 2022-08-05T08:43:49Z

releasenotes/notes/fix-hinton-bug-1141a297050f55bb.yaml

+    Fix known bug in plot_state_hinton where axes are rendered improperly.
+fixes:
+  - |
+    Fix known bug in plot_state_hinton. Refer to `#8446 <https://github.com/Qiskit/qiskit-terra/issues/8446>` for more details.


Can you please describe the bug in a bit more details here? You can link to the issue but it's good to have a quick summary here. If you write :func: ~Qiskit.tools.visualization.plot_state_hinton, when the release notes are rendered to the webpage, it would automatically link to the function documentation.

Thanks for the help. I removed the "issues" section and added more details to "fixes".

HuangJunye · 2022-08-09T16:20:26Z

I corrected the ylim part which was a mistake introduced same PR that caused the issue this PR (8447) is trying to fix by #4811.

4811 mistakenly did ax1.set_ylim([ly, 0]) twice which basically cancel each other. And ax1.invert_yaxis() inverts the axis. On the other hand, ylim of ax2 was not set, and ax2.invert_yaxis() inverts the axis. The end results is that both y axis are inverted which are the desired plotting method.

When @wyqian1027 corrected ax1.set_ylim([ly,0]) to ax2.set_ylim([ly,0]), the inverted ylim and the invert axis cancel each other, that's why the y axis was back to normal order (lower values at the bottom)

HuangJunye

LGTM. Thanks a lot for the fix. Congrats for getting your first PR merged! Hope you can continue making contributions to Qiskit. If you need any help in the future, feel free to reach out to me on GitHub or Qiskit Slack.

wyqian1027 · 2022-08-09T18:00:26Z

LGTM. Thanks a lot for the fix. Congrats for getting your first PR merged! Hope you can continue making contributions to Qiskit. If you need any help in the future, feel free to reach out to me on GitHub or Qiskit Slack.

Thank you! I will do my best.

HuangJunye · 2022-08-11T17:32:20Z

Looks like CI failed due to server errors. Can we trigger the CI to run again? @mtreinish

jakelishman

I'm concerned about the change in ordering the outputs here. It's not simply a style choice (and even if it were, we already picked a convention, and it's not good for us to change that without a very good reason). The original form makes the Hinton plot resemble the standard matrix order, which makes it easier to read.

HuangJunye · 2022-08-17T07:50:38Z

@jakelishman I reverted the y-axis direction in 4b43d59. It should be the same as the original direction.

HuangJunye · 2022-08-17T07:54:48Z

Related to that, currently the hinton plot seems to plot the matrix transversely. Is that the expected behaviour?

HuangJunye · 2022-08-22T08:22:11Z

@jakelishman

I'm concerned about the change in ordering the outputs here. It's not simply a style choice (and even if it were, we already picked a convention, and it's not good for us to change that without a very good reason). The original form makes the Hinton plot resemble the standard matrix order, which makes it easier to read.

I reverted the y-axis direction in 4b43d59. It should be the same as the original direction. Do you think the PR is good for merging?

The state labels in `plot_state_hinton` did not align with the correct matrix entries. This also updates the plot to display entries in the natural "matrix" ordering, i.e. the same way that Numpy would display the matrix if directly printed. Previously, it was effectively a transpose of what it should have been, with incorrect labels.

jakelishman

Sorry for the major delay here!

I fixed up the bit labels to match up with Qiskit's conventions, and so that DensityMatrix.from_label(...) will now look "the same" whether you print out its data matrix, or plot it on the Hinton plot. For example:

In [1]: from qiskit.visualization import plot_state_hinton
   ...:
   ...: arr = np.array([[1, 0, 0, 0], [0, 0, np.exp(1j * np.pi/4), 0], [0, np.exp(1j * 5*np.pi/4), 0, 0], [0, 0, 0, 1]])
   ...: plot_state_hinton(arr)
   ...: arr
Out[1]:
array([[ 1.        +0.j        ,  0.        +0.j        ,  0.        +0.j        ,  0.        +0.j        ],
       [ 0.        +0.j        ,  0.        +0.j        ,  0.70710678+0.70710678j,  0.        +0.j        ],
       [ 0.        +0.j        , -0.70710678-0.70710678j,  0.        +0.j        ,  0.        +0.j        ],
       [ 0.        +0.j        ,  0.        +0.j        ,  0.        +0.j        ,  1.        +0.j        ]])

now gives

Previously, the squares in the middle would be transposed relative to how the matrix prints, which is confusing (but what Matplotlib's documentation does, at least at the time of writing, which is where this function originally came from).

jakelishman · 2022-10-18T22:28:36Z

@Mergifyio requeue

mergify · 2022-10-18T22:28:45Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

jakelishman · 2022-10-18T22:29:56Z

@Mergifyio refresh

mergify · 2022-10-18T22:30:30Z

refresh

✅ Pull request refreshed

* fix hinton visualization bug * added release note for fix-hinton-bug * remove issues and add more details * correct set ylim * Fix display and labelling of Hinton plot The state labels in `plot_state_hinton` did not align with the correct matrix entries. This also updates the plot to display entries in the natural "matrix" ordering, i.e. the same way that Numpy would display the matrix if directly printed. Previously, it was effectively a transpose of what it should have been, with incorrect labels. * Fixup release note Co-authored-by: Junye Huang <h.jun.ye@gmail.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a53fc9a)

* fix hinton visualization bug * added release note for fix-hinton-bug * remove issues and add more details * correct set ylim * Fix display and labelling of Hinton plot The state labels in `plot_state_hinton` did not align with the correct matrix entries. This also updates the plot to display entries in the natural "matrix" ordering, i.e. the same way that Numpy would display the matrix if directly printed. Previously, it was effectively a transpose of what it should have been, with incorrect labels. * Fixup release note Co-authored-by: Junye Huang <h.jun.ye@gmail.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a53fc9a) Co-authored-by: WQ <42972544+wyqian1027@users.noreply.github.com>

HuangJunye · 2022-10-20T09:35:46Z

@jakelishman Thanks for following up and checking it thoroughly!

fix hinton visualization bug

5b1a8c9

wyqian1027 requested review from a team, nonhermitian and nkanazawa1989 as code owners August 4, 2022 08:48

Merge branch 'main' into fix_hinton_bug

6b0f997

1ucian0 added documentation Something is not clear or an error documentation Community PR PRs from contributors that are not 'members' of the Qiskit repo labels Aug 4, 2022

HuangJunye self-assigned this Aug 4, 2022

Merge branch 'main' into fix_hinton_bug

cb3dcc2

wyqian1027 and others added 3 commits August 4, 2022 21:39

added release note for fix-hinton-bug

39c8996

Merge branch 'main' into fix_hinton_bug

47e9ce5

Merge branch 'fix_hinton_bug' of github.com:wyqian1027/Fork__qiskit-t…

e92c562

…erra into fix_hinton_bug

HuangJunye reviewed Aug 5, 2022

View reviewed changes

wyqian1027 and others added 2 commits August 5, 2022 17:39

remove issues and add more details

689325e

correct set ylim

4b43d59

HuangJunye previously approved these changes Aug 9, 2022

View reviewed changes

HuangJunye mentioned this pull request Aug 9, 2022

Plot_hinton imaginary section displays incorrect elements #8324

Closed

Merge branch 'main' into fix_hinton_bug

ae83352

jakelishman requested changes Aug 16, 2022

View reviewed changes

HuangJunye and others added 4 commits August 23, 2022 09:50

Merge branch 'main' into fix_hinton_bug

90a56e6

Fixup release note

44432e2

Merge remote-tracking branch 'ibm/main' into fix_hinton_bug

4e3aa07

jakelishman dismissed HuangJunye’s stale review via 4e3aa07 October 18, 2022 17:00

jakelishman approved these changes Oct 18, 2022

View reviewed changes

jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable automerge mod: visualization qiskit.visualization and removed documentation Something is not clear or an error documentation labels Oct 18, 2022

Merge branch 'main' into fix_hinton_bug

36c9f21

Merge branch 'main' into fix_hinton_bug

46666a7

mergify bot merged commit a53fc9a into Qiskit:main Oct 19, 2022

mergify bot mentioned this pull request Oct 19, 2022

fix hinton visualization bug (backport #8447) #8946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix hinton visualization bug #8447

fix hinton visualization bug #8447

wyqian1027 commented Aug 4, 2022 •

edited by HuangJunye

Loading

qiskit-bot commented Aug 4, 2022

CLAassistant commented Aug 4, 2022 •

edited

Loading

CLAassistant commented Aug 4, 2022

HuangJunye commented Aug 4, 2022 •

edited

Loading

HuangJunye commented Aug 4, 2022

coveralls commented Aug 4, 2022 •

edited

Loading

wyqian1027 commented Aug 4, 2022 •

edited

Loading

HuangJunye left a comment

HuangJunye Aug 5, 2022

HuangJunye Aug 5, 2022

wyqian1027 Aug 5, 2022

HuangJunye commented Aug 9, 2022

HuangJunye left a comment

wyqian1027 commented Aug 9, 2022

HuangJunye commented Aug 11, 2022

jakelishman left a comment

HuangJunye commented Aug 17, 2022

HuangJunye commented Aug 17, 2022

HuangJunye commented Aug 22, 2022

jakelishman left a comment

jakelishman commented Oct 18, 2022

mergify bot commented Oct 18, 2022

jakelishman commented Oct 18, 2022

mergify bot commented Oct 18, 2022

HuangJunye commented Oct 20, 2022

	issues:
	- \|
	Fix known bug in plot_state_hinton where axes are rendered improperly.

fix hinton visualization bug #8447

fix hinton visualization bug #8447

Conversation

wyqian1027 commented Aug 4, 2022 • edited by HuangJunye Loading

Summary

Details and comments

qiskit-bot commented Aug 4, 2022

CLAassistant commented Aug 4, 2022 • edited Loading

CLAassistant commented Aug 4, 2022

HuangJunye commented Aug 4, 2022 • edited Loading

HuangJunye commented Aug 4, 2022

coveralls commented Aug 4, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3277537504

💛 - Coveralls

wyqian1027 commented Aug 4, 2022 • edited Loading

HuangJunye left a comment

Choose a reason for hiding this comment

HuangJunye Aug 5, 2022

Choose a reason for hiding this comment

HuangJunye Aug 5, 2022

Choose a reason for hiding this comment

wyqian1027 Aug 5, 2022

Choose a reason for hiding this comment

HuangJunye commented Aug 9, 2022

HuangJunye left a comment

Choose a reason for hiding this comment

wyqian1027 commented Aug 9, 2022

HuangJunye commented Aug 11, 2022

jakelishman left a comment

Choose a reason for hiding this comment

HuangJunye commented Aug 17, 2022

HuangJunye commented Aug 17, 2022

HuangJunye commented Aug 22, 2022

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman commented Oct 18, 2022

mergify bot commented Oct 18, 2022

❌ This pull request head commit has not been previously disembarked from queue.

jakelishman commented Oct 18, 2022

mergify bot commented Oct 18, 2022

✅ Pull request refreshed

HuangJunye commented Oct 20, 2022

wyqian1027 commented Aug 4, 2022 •

edited by HuangJunye

Loading

CLAassistant commented Aug 4, 2022 •

edited

Loading

HuangJunye commented Aug 4, 2022 •

edited

Loading

coveralls commented Aug 4, 2022 •

edited

Loading

wyqian1027 commented Aug 4, 2022 •

edited

Loading