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

Remove visualization code deprecated in 0.22 (released on October 13, 2022) #10989

Merged
merged 14 commits into from
Jan 29, 2024

Conversation

ThirDecade2020
Copy link
Contributor

@ThirDecade2020 ThirDecade2020 commented Oct 7, 2023

Summary

Fixes #10812

  • Remove deprecated code and tests (if they only check the deprecation raise).
  • Tag the PR as Changelog: Removal.
  • Create a release note in the category upgrade:. Include an example with an alternative for user to migrate to the new code, as this change might break users code.

Details and comments

I am not exactly sure how to remove all deprecated tests if they only check the deprecation raise and then add tests to cover changes.

How do I label and create release note properly?

See 'data argument description' for "Migration Guide" instructions.

Remove deprecated code:

qiskit/visualization/circuit/_utils.py:200
qiskit/visualization/circuit/_utils.py:288
There is code released 9 months ago that needs to be removed.

qiskit/visualization/counts_visualization.py:60

Include an example with an alternative for user to migrate to the new code, as this change might break users code.
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Oct 7, 2023
@CLAassistant
Copy link

CLAassistant commented Oct 7, 2023

CLA assistant check
All committers have signed the CLA.

@1ucian0 1ucian0 changed the title Issue #10812 Remove Deprecated Code & Tests, & Provide Alternative Remove visualization code deprecated in 0.22 (released on October 13, 2022) Oct 10, 2023
@1ucian0 1ucian0 added the Changelog: Removal Include in the Removed section of the changelog label Oct 10, 2023
@1ucian0
Copy link
Member

1ucian0 commented Oct 10, 2023

Some quick comments before having a deeper look:

  • I ran make black to fix the current CI issue.
  • Pylint is failing with the following (you can have a look at it)
qiskit/visualization/counts_visualization.py:23:0: W0611: Unused deprecate_arg imported from qiskit.utils.deprecation (unused-import)
  • I added the PR label (no worries, that one is on us)
  • you can create the release note with reno new fix_10989

@1ucian0
Copy link
Member

1ucian0 commented Dec 12, 2023

Hi @ThirDecade2020 , are you still working on this one?

@coveralls
Copy link

coveralls commented Jan 17, 2024

Pull Request Test Coverage Report for Build 7642428329

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 89.516%

Totals Coverage Status
Change from base Build 7586004387: 0.05%
Covered Lines: 59545
Relevant Lines: 66519

💛 - Coveralls

@hunterkemeny
Copy link
Contributor

@1ucian0 @mtreinish I can't figure out why docs is failing in the CI, any ideas?

@mtreinish
Copy link
Member

The root of the docs failure is formatting issues in the plot_histogram , the specific lines in the log are: https://dev.azure.com/qiskit-ci/qiskit/_build/results?buildId=53504&view=logs&j=a15d9aca-d75c-55b1-7f76-889cfe884cf0&t=154f6391-7dd7-5733-2230-2f10f0798b76&l=568 (note the line numbers are relative to the plot_histogram docstring). I'll leave some inline comments on how to fix the formatting.

qiskit/visualization/counts_visualization.py Outdated Show resolved Hide resolved
qiskit/visualization/counts_visualization.py Outdated Show resolved Hide resolved
qiskit/visualization/counts_visualization.py Outdated Show resolved Hide resolved
@hunterkemeny hunterkemeny marked this pull request as ready for review January 19, 2024 15:44
@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:

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.

Could you also add a reno? 🙂

qiskit/visualization/counts_visualization.py Show resolved Hide resolved
qiskit/visualization/counts_visualization.py Show resolved Hide resolved
qiskit/visualization/counts_visualization.py Outdated Show resolved Hide resolved
@mtreinish mtreinish added this to the 1.0.0 milestone Jan 27, 2024
@mtreinish mtreinish self-assigned this Jan 29, 2024
@hunterkemeny hunterkemeny added this pull request to the merge queue Jan 29, 2024
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, there's some minor formatting things in the release notes that I'd clean up. But, given the proximity to RC1 lets save that for later, and we can fix that up in the release note roundup PR for the final release.

Merged via the queue into Qiskit:main with commit fefa722 Jan 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Removal Include in the Removed section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove code deprecated in 0.22 (released on October 13, 2022) [visualization]
8 participants