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

deprecate legacy pulse builder command #11249

Conversation

MozammilQ
Copy link
Contributor

@MozammilQ MozammilQ commented Nov 15, 2023

Summary

This addresses issue #11152

This PR

Deprecates

  • qiskit.pulse.builder.call_gate

  • qiskit.pulse.builder.cx

  • qiskit.pulse.builder.u1

  • qiskit.pulse.builder.u2

  • qiskit.pulse.builder.u3

  • qiskit.pulse.builder.x

  • qiskit.pulse.builder.active_transpiler_settings

  • qiskit.pulse.builder.active_circuit_scheduler_settings

  • qiskit.pulse.builder.transpiler_settings

  • arguments default_transpiler_settings ,
    and default_circuit_scheduler_settings of
    qiskit.pulse.builder.build function.

  • QuantumCircuit type of the target argument in qiskit.pulse.builder.call function

Modifies:

  • Module doc of qiskit.pulse.builder.py (example code with circuit elements)
  • The example code 3 of function doc for qiskit.pulse.builder.call

and, modifies related tests in

  • test/python/pulse/test_builder.py
  • test/python/pulse/test_block.py
  • test/python/pulse/test_builder_v2.py
  • test/python/transpiler/test_calibrationbuilder.py

Details and comments

Refer to Issue: #11152
fixes #11152

@MozammilQ MozammilQ requested review from eggerdj, wshanks and a team as code owners November 15, 2023 15:27
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 15, 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

@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , please see if this is good enough :)

Copy link
Collaborator

@TsafrirA TsafrirA left a comment

Choose a reason for hiding this comment

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

Thanks @MozammilQ for opening this PR.

I believe the deprecation message and the release notes should be clearer in conveying the alternative we want the users to use. While it is possible to extract the calibration from the Target and add it to the ScheduleBlock, the thing we want them to do is actually to move this logic into the circuit portion of their jobs. So, instead of having pulse.cx we want them to use circuit.cx outside of the pulse builder, as part of the circuit.

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Nov 20, 2023

Thanks Tsafrir. That's good point. I agree we should encourage users to write their program with QuantumCircuit model and call non-circuit part as a pulse gate. @MozammilQ could you please rephrase the waring message? Something like

Use QuantumCircuit with XGate instead, and attach the rest of pulse sequence as a pulse gate

should be okey.

@nkanazawa1989 nkanazawa1989 added mod: pulse Related to the Pulse module Changelog: Deprecation Include in "Deprecated" section of changelog labels Nov 20, 2023
@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , please see if this is good enough :)

Copy link
Collaborator

@TsafrirA TsafrirA left a comment

Choose a reason for hiding this comment

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

This is much better. Still needs some more careful wording. Some of the docs haven't been adjusted, I will try to highlight them separately.

qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved
@TsafrirA
Copy link
Collaborator

The example here should be removed.

@MozammilQ
Copy link
Contributor Author

MozammilQ commented Nov 23, 2023

@TsafrirA ,
Applied all suggestions :)
I have done exactly as you have suggested!
I am afraid, I would have ever considered going this verbose on deprecation messages which was a mistake that I will take care of next time :)

@nkanazawa1989

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Deprecation for circuit_scheduler_settings command is missing. Apart from this, the PR looks good to go.

@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , please see if this is good enough :)

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Sorry I didn't carefully check the release note. Here is additional suggestions to make it better.

@MozammilQ
Copy link
Contributor Author

@nkanazawa1989 , please see if this is good enough :)

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @MozammilQ

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Dec 18, 2023
Merged via the queue into Qiskit:stable/0.46 with commit 4c073de Dec 18, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: pulse Related to the Pulse module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants