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.

Sign up for GitHub

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

Jump to bottom

Remove deprecated ParametricPulse classes #11024

Merged
merged 11 commits into from
Dec 7, 2023
Merged

Remove deprecated ParametricPulse classes #11024

merged 11 commits into from
Dec 7, 2023

Conversation

Ak-ash22
Copy link
Contributor

@Ak-ash22 Ak-ash22 commented Oct 16, 2023 •
edited
Loading

Summary

Fixes #10810

Details and comments

Removed deprecated code parametric_pulses.py and tests. I noticed a bug in the related test.... in the qiskit/test/python/pulse/test_pulse_lib.py, there is a test class named TestParametricPulses(..) but it calls all the Gaussian, Constant and other waveforms from the symbolic_pulses.py and not from the paramteric_pulses.py. So I changed the name of the test class to TestSymbolicPulses(..).
Removed the related code from other files where ParametricPulses class was imported and used.
Created a reno 'remove-deprecated-parametric-pulses-class-667e4b970e1163b3.yaml' for the issue.

This PR has been requested to tagged as Changelog: Removal to reflect the removal deprecated components.

@Ak-ash22 Ak-ash22 requested review from nonhermitian, a team, eggerdj and wshanks as code owners October 16, 2023 16:11
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Oct 16, 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

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Oct 24, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 7124792193

  • 8 of 9 (88.89%) changed or added relevant lines in 7 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 87.567%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/pulse_v2/core.py 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.67%
Totals Coverage Status
Change from base Build 7108045183: 0.1%
Covered Lines: 59843
Relevant Lines: 68340
💛 - Coveralls

@1ucian0 1ucian0 self-assigned this Oct 25, 2023
@Ak-ash22
Copy link
Contributor Author

Ak-ash22 commented Nov 7, 2023

@1ucian0 Its been a while, can you help me get the review from the code owners?

rupeshknn
rupeshknn approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@rupeshknn rupeshknn left a comment

Choose a reason for hiding this comment

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

LGTM

qiskit/qobj/converters/pulse_instruction.py Outdated
@@ -50,12 +50,12 @@ class ParametricPulseShapes(Enum):
@classmethod
def from_instance(
cls,
instance: Union[library.ParametricPulse, library.SymbolicPulse],
instance: [library.SymbolicPulse],
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need the square brackets here

@wshanks wshanks changed the title Fixissue10810 Remove deprecated ParametricPulse classes Nov 15, 2023
@wshanks wshanks mentioned this pull request Nov 20, 2023
Closed
@TsafrirA TsafrirA mentioned this pull request Nov 21, 2023
Merged
@nkanazawa1989
Copy link
Contributor

Hi @Ak-ash22 we had another PR but the author @TsafrirA suggested to merge yours. Can you resolve conflicts?

@nkanazawa1989 nkanazawa1989 added the mod: pulse Related to the Pulse module label Nov 27, 2023
@Ak-ash22
Copy link
Contributor Author

Yes sure!.. I'll resolve it

@Ak-ash22
Copy link
Contributor Author

@1ucian0 @nkanazawa1989 I have resolved all the conflicts and submitted it for PR. Kindly, take a look and help me merge it. Thank You!

@1ucian0 1ucian0 added the Changelog: Removal Include in the Removed section of the changelog label Dec 4, 2023
1ucian0
1ucian0 reviewed Dec 4, 2023
View reviewed changes
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 a lot for your work @Ak-ash22 !

it looks good to me in general (just some comments in the release note). However, it would be great if @nkanazawa1989 can have the final 👍 .

(and thanks @rupeshknn for reviewing it ahead!)

...tes/notes/releasenotes/notes/remove-deprecated-parametric-pulses-class-667e4b970e1163b3.yaml Outdated Show resolved Hide resolved
...tes/notes/releasenotes/notes/remove-deprecated-parametric-pulses-class-667e4b970e1163b3.yaml Outdated Show resolved Hide resolved
qiskit/pulse/library/symbolic_pulses.py
@@ -1465,8 +1465,7 @@ def __new__(
# Note this is implemented using Piecewise instead of just returning amp
# directly because otherwise the expression has no t dependence and sympy's
# lambdify will produce a function f that for an array t returns amp
# instead of amp * np.ones(t.shape). This does not work well with
# ParametricPulse.get_waveform().
# instead of amp * np.ones(t.shape).
Copy link
Member

Choose a reason for hiding this comment

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

great catch fixing the comments!

...tes/notes/releasenotes/notes/remove-deprecated-parametric-pulses-class-667e4b970e1163b3.yaml Outdated Show resolved Hide resolved
Ak-ash22 and others added 3 commits December 4, 2023 19:14
@Ak-ash22 @1ucian0
Update releasenotes/notes/releasenotes/notes/remove-deprecated-parame…
ab16d3a 
…tric-pulses-class-667e4b970e1163b3.yaml

Co-authored-by: Luciano Bello <[email protected]>
@Ak-ash22 @1ucian0
Update releasenotes/notes/releasenotes/notes/remove-deprecated-parame…
21e2b8e 
…tric-pulses-class-667e4b970e1163b3.yaml

Co-authored-by: Luciano Bello <[email protected]>
@Ak-ash22 @1ucian0
Update releasenotes/notes/releasenotes/notes/remove-deprecated-parame…
22a365a 
…tric-pulses-class-667e4b970e1163b3.yaml

Co-authored-by: Luciano Bello <[email protected]>
@nkanazawa1989
Copy link
Contributor

Thanks @Ak-ash22 this looks good to me!

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Dec 7, 2023
Merged via the queue into Qiskit:main with commit ac7aca3 Dec 7, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ucian0 1ucian0 1ucian0 left review comments

@nkanazawa1989 nkanazawa1989 nkanazawa1989 approved these changes

@rupeshknn rupeshknn rupeshknn approved these changes

@nonhermitian nonhermitian Awaiting requested review from nonhermitian nonhermitian is a code owner

@eggerdj eggerdj Awaiting requested review from eggerdj eggerdj is a code owner

@wshanks wshanks Awaiting requested review from wshanks wshanks is a code owner

Assignees

@1ucian0 1ucian0

Labels
Changelog: Removal Include in the Removed section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: pulse Related to the Pulse module
Projects
Contributor Monitoring
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove code deprecated in 0.22 (released on October 13, 2022) [pulse]
7 participants
@Ak-ash22 @qiskit-bot @CLAassistant @coveralls @nkanazawa1989 @1ucian0 @rupeshknn