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

Removed deprecated code since 0.23 #11188

Closed
wants to merge 4 commits into from
Closed

Removed deprecated code since 0.23 #11188

wants to merge 4 commits into from

Conversation

neet-14
Copy link
Contributor

@neet-14 neet-14 commented Nov 3, 2023

Summary

fixes issue #11144
Changelog: Removal

Details and comments

@1ucian0

@neet-14 neet-14 requested review from nonhermitian and a team as code owners November 3, 2023 07:09
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 3, 2023
@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:

  • @Qiskit/terra-core
  • @nkanazawa1989

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6755266803

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 86.949%

Files with Coverage Reduction New Missed Lines %
qiskit/visualization/pulse/matplotlib.py 1 0.0%
crates/qasm2/src/lex.rs 2 91.16%
Totals Coverage Status
Change from base Build 6751909510: 0.02%
Covered Lines: 74366
Relevant Lines: 85528

💛 - Coveralls

@ElePT ElePT added the Changelog: Removal Include in the Removed section of the changelog label Nov 6, 2023
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks for giving a try to this deprecation removal, but the current changes are not doing what we were intending with the original issue. Maybe I can help clarify the purpose of this PR.

If you look at the implementation of the deprecation decorators, you will notice that @deprecate_func on top of a class __init__ indicates that the full class is deprecated (not only the __init__). The goal of this PR is to remove the deprecated classes, i.e., EventsOutputChannels, WaveformDrawer and ScheduleDrawer. Given that these are the only classes in qiskit/visualization/pulse/matplotlib.py, the easiest way to go about it is to actually remove the file. If you look at all of the classes mentioned in #11144, you will notice that actually all of the classes in qiskit/visualization/pulse are deprecated and should be removed. I would suggest merging the changes of this PR and #11167 into one single PR where you delete qiskit/visualization/pulse and any related tests.

You will then have to make sure that any references to these classes on the documentation (probably on the visualization __init__.py file) are also removed, or the documentation build will fail on the automated checks. Finally, you should document what you did on the release note, that is, you should explain that these classes have been removed from the codebase, that they were deprecated on qiskit-tera 0.23, and that their functionality (pulse visualization) was replaced by the interface inqiskit.visualization.pulse_drawer.

@@ -0,0 +1,5 @@
---
Copy link
Contributor

@ElePT ElePT Nov 6, 2023

Choose a reason for hiding this comment

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

One more note, it is very helpful for us if the release notes have descriptive names, such as remove-mpl-pulse (the important part is that it says "remove" and what it is removing). This is not a hard requirement, but as I said, helpful to later locate the relevant release notes. [Edit: if you apply my suggestion above, the title should be something like remove-deprecated-visualization-pulse]

@ElePT
Copy link
Contributor

ElePT commented Nov 21, 2023

Closing this PR in favor of #11213

@ElePT ElePT closed this Nov 21, 2023
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.

5 participants