Skip to content

Remove ParametricPulse #11281

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

Closed
wants to merge 1 commit into from
Closed

Conversation

TsafrirA
Copy link
Collaborator

Summary

This PR removes the ParametericPulse base class and pulse library.

Details and comments

The ParametricPulse library has been superseded by the SymbolicPulse library which provides QPY support and some performance improvements. It is supposed to be deprecated in Qiskit 0.46 in #11279, and has been pending deprecation since Terra 0.22. This PR removes the base class and pulse library.

@qiskit-bot
Copy link
Collaborator

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 6928112990

  • 8 of 9 (88.89%) changed or added relevant lines in 7 files are covered.
  • 17 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.08%) to 85.966%

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 5 91.92%
crates/qasm2/src/parse.rs 12 97.13%
Totals Coverage Status
Change from base Build 6924648858: 0.08%
Covered Lines: 65749
Relevant Lines: 76483

💛 - Coveralls

Copy link
Contributor

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

This looks fine but note that #11024 already does this. I put a comment in-line based on something I saw there but not here.

) -> "ParametricPulseShapes":
"""Get Qobj name from the pulse class instance.

Args:
instance: Symbolic or ParametricPulse class.
Copy link
Contributor

Choose a reason for hiding this comment

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

The references to library.parametric_pulses below should be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks.

@TsafrirA
Copy link
Collaborator Author

@wshanks @nkanazawa1989
Thanks @wshanks for the heads up. I forgot about #11024. It came first, and it's better to promote community contribution, so I am absolutely fine with going with that PR.

@@ -257,8 +256,6 @@ def _build_waveform(self):
for command in commands:
duration = command.duration
tf = min(time + duration, self.tf)
if isinstance(command, ParametricPulse):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to replace this with SymbolicPulse?

def detail_title(
program: Union[pulse.Waveform, pulse.ParametricPulse, pulse.Schedule], device: DrawerBackendInfo
) -> str:
def detail_title(program: Union[pulse.Waveform, pulse.Schedule], device: DrawerBackendInfo) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here


The ``ParametricPulse`` base class and pulse library were superseded by the
:class:`~qiskit.pulse.SymbolicPulse` class and pulse library, which provide
better performance and QPY support. All removed library pulses have a
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if symbolic will provide better performance. Visualization is usually slow due to symbolic expression evaluation overhead.

@TsafrirA
Copy link
Collaborator Author

@nkanazawa1989 following my previous comment, do you want to handle this here or on #11024?

@ElePT ElePT modified the milestone: 1.0.0 Nov 21, 2023
@nkanazawa1989 nkanazawa1989 added the mod: pulse Related to the Pulse module label Nov 27, 2023
@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Nov 27, 2023

Close in favor of #11024 (can be reopened if another PR cannot meet 1.0 deadline)

@TsafrirA TsafrirA deleted the RemoveParametricPulse branch February 7, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: pulse Related to the Pulse module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants