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

Plot State City Visualization Fixes #10590

Merged
merged 15 commits into from
Oct 26, 2023
Merged

Plot State City Visualization Fixes #10590

merged 15 commits into from
Oct 26, 2023

Conversation

AlexanderGroeger
Copy link
Contributor

@AlexanderGroeger AlexanderGroeger commented Aug 8, 2023

This is my first PR, please correct me as needed. :)

Summary

The qiskit.visualization module has a function plot_state_city which yields a matplotlib 3d bar chart representing the density matrix of a quantum state.

Fixes #8413
Fixes #3315
Related to #10862

Details and comments

Note for some extreme choices of figsize, the plot labels can still be cut off. The primary issue is matplotlib cannot use tight_layout because there's a lack of space.

@AlexanderGroeger AlexanderGroeger requested review from nonhermitian and a team as code owners August 8, 2023 16:59
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 8, 2023
@CLAassistant
Copy link

CLAassistant commented Aug 8, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
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.

@qiskit-bot
Copy link
Collaborator

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:

@AlexanderGroeger
Copy link
Contributor Author

AlexanderGroeger commented Aug 8, 2023

Looks like the qiskit-terra test failed on Run image test, specifically test_plot_state_city did not succeed. Apparently the error was AssertionError: 0.930396 not greater than or equal to 0.99 coming from

ratio = VisualTestUtilities._save_diff(
            self._image_path(fname),
            self._reference_path(fname),
            fname,
            FAILURE_DIFF_DIR,
            FAILURE_PREFIX,
        )
        self.assertGreaterEqual(ratio, 0.99)

@Cryoris
Copy link
Contributor

Cryoris commented Aug 15, 2023

Since your PR changes the output of the plot_state_city function, you probably have to change the reference pictures in used in the tests. See e.g. the comments of #10619 on how to do this 🙂

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5799800653

  • 0 of 59 (0.0%) changed or added relevant lines in 1 file are covered.
  • 25 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.01%) to 87.256%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/state_visualization.py 0 59 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/squ.py 2 80.0%
qiskit/visualization/state_visualization.py 2 58.72%
crates/qasm2/src/lex.rs 3 90.63%
crates/qasm2/src/parse.rs 18 96.67%
Totals Coverage Status
Change from base Build 5787606820: 0.01%
Covered Lines: 74240
Relevant Lines: 85083

💛 - Coveralls

@AlexanderGroeger
Copy link
Contributor Author

I believe this PR should be ready for review, sorry about the multiple merges; I wasn't sure if I should keep my PR up-to-date or not.

@AlexanderGroeger
Copy link
Contributor Author

If this PR could be reviewed and closed soon, that would be great. Thanks!

@1ucian0 1ucian0 self-assigned this Sep 21, 2023
@1ucian0 1ucian0 added this to the 0.45.0 milestone Sep 21, 2023
@jakelishman jakelishman added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Oct 10, 2023
@1ucian0
Copy link
Member

1ucian0 commented Oct 16, 2023

Thanks a lot for your contribution. Original, the bug in #10862 was introduced by a rounding error that made testing. Do you know if your changes are sensitive to them?

I never understood why there is a color difference for 0.0:
state_city

The RGB values shouldn't be the same?

@AlexanderGroeger
Copy link
Contributor Author

Regarding a rounding error that made testing, I removed this line altogether without using any substitute code. I didn't quite understand why this was an issue. It sounded like bars with zero height were considered negative in the visualization, but I'm wondering if this was just a problem with how the dividing zero plane was plotted in which that has been corrected.
The reason for the color difference eludes me too. I was initially considering the issue was with generate_facecolors, but there is nothing obvious there. I'm starting to wonder if it related to how matplotlib handles 3d projections.

@1ucian0
Copy link
Member

1ucian0 commented Oct 17, 2023

Is this fixing #8413 , #3315 and #10862 ? If so uses Fixes #<number> in the main message for close them automatically once this gets merged.

@AlexanderGroeger
Copy link
Contributor Author

Is this fixing #8413 , #3315 and #10862 ? If so uses Fixes #<number> in the main message for close them automatically once this gets merged.

Yes. I edited the PR description to reflect this. I'm still a bit new to some of these expectations, so thank you!

@1ucian0
Copy link
Member

1ucian0 commented Oct 18, 2023

Take a look to the black issue.

Dropping the label for the axes (Re and Im) was intentional?

@AlexanderGroeger
Copy link
Contributor Author

Yes. I figured putting subtitles over each subplot would be easier to read instead. The primarily reason why the y-axis labels were moved was because of spacing issues related to #8413.

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Oct 25, 2023
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!

@1ucian0 1ucian0 modified the milestones: 0.45.0, 0.45.1 Oct 26, 2023
@1ucian0 1ucian0 added this pull request to the merge queue Oct 26, 2023
Merged via the queue into Qiskit:main with commit 4b5546f Oct 26, 2023
13 checks passed
mergify bot pushed a commit that referenced this pull request Oct 26, 2023
* fix zordering and labels outside of plot

* lint and reno

* Changed reference image for state city

* correct visual of negative real value bars

* added release notes for negative real bars fix

* remove debug print statement in plot_state_city

* fix with tox eblack

* append rho to title

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
(cherry picked from commit 4b5546f)
github-merge-queue bot pushed a commit that referenced this pull request Oct 30, 2023
* fix zordering and labels outside of plot

* lint and reno

* Changed reference image for state city

* correct visual of negative real value bars

* added release notes for negative real bars fix

* remove debug print statement in plot_state_city

* fix with tox eblack

* append rho to title

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
(cherry picked from commit 4b5546f)

Co-authored-by: AlexanderGroeger <46076580+AlexanderGroeger@users.noreply.github.com>
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 Community PR PRs from contributors that are not 'members' of the Qiskit repo priority: medium stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Status: Done
8 participants