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

Refactor state latex drawer #8874

Merged
merged 14 commits into from
Jan 17, 2023
Merged

Refactor state latex drawer #8874

merged 14 commits into from
Jan 17, 2023

Conversation

frankharkins
Copy link
Member

@frankharkins frankharkins commented Oct 11, 2022

Summary

Currently, we have two different algorithms to display amplitudes as human-readable latex strings. This PR adapts the functions in qiskit/visualization/array.py so they work for both drawers, and deprecates the duplicate drawer.

Details and comments

  • I went with the sympy approach of the ket drawer over the home-rolled algorithm in the array drawer.
  • As a side effect, the array drawer may behave slightly differently, I've mentioned this in the releasenotes. The precision default in the matrix drawer needed increasing to continue recognising common amplitudes. The ket drawer should produce latex that renders identically so hopefully minimal disruption there.
  • I've also deprecated some public functions (num_to_latex_ket, numbers_to_latex_terms) as they're quite specific to the drawer and I don't think it falls under Qiskit's scope to support them. I can't find any uses of them in the wild.

This PR supersedes #8551

@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:

@coveralls
Copy link

coveralls commented Oct 11, 2022

Pull Request Test Coverage Report for Build 3940769746

  • 43 of 49 (87.76%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 84.859%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/array.py 27 33 81.82%
Files with Coverage Reduction New Missed Lines %
qiskit/visualization/array.py 1 86.25%
src/sabre_swap/layer.rs 2 98.95%
Totals Coverage Status
Change from base Build 3940320475: 0.03%
Covered Lines: 65799
Relevant Lines: 77539

💛 - Coveralls

Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

Thank you. The code is more concise and the output is easier to read. I will leave some minor comments.
image

releasenotes/notes/latex-refactor-0745471ddecac605.yaml Outdated Show resolved Hide resolved
qiskit/visualization/state_visualization.py Outdated Show resolved Hide resolved
qiskit/visualization/array.py Outdated Show resolved Hide resolved
@ikkoham ikkoham added Changelog: Deprecation Include in "Deprecated" section of changelog mod: visualization qiskit.visualization labels Nov 22, 2022
ikkoham
ikkoham previously approved these changes Nov 22, 2022
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my review comments. LGTM

@ikkoham ikkoham added this to the 0.23.0 milestone Nov 24, 2022
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.

I reworded the release note a little, but otherwise this looks fine to me to merge. It's good to consolidate all the code in one place for consistency, and if it reduces some of the spaghetti back-and-forth we'd grown over time, I'm all for it. Thanks!

@jakelishman jakelishman added Changelog: API Change Include in the "Changed" section of the changelog automerge labels Jan 17, 2023
@mergify mergify bot merged commit c2215ae into Qiskit:main Jan 17, 2023
@frankharkins frankharkins deleted the latex-refactor branch January 18, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: Deprecation Include in "Deprecated" section of changelog mod: visualization qiskit.visualization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants