-
Notifications
You must be signed in to change notification settings - Fork 127
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 pulse and calibration management related code #1511
Conversation
a15e668
to
61dcf0e
Compare
Also, remove use of alignment arguments from BackendTiming
93b8618
to
12d1da8
Compare
Still to be done:
Could be deferred to later PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hard work. The PR seems to cover all necessary changes we need to make for Qiskit 2.0.
We also need to consider the migration of this tutorial:
https://qiskit-community.github.io/qiskit-dynamics/tutorials/dynamics_backend.html
Qiskit Dynamics decided to pin Qiskit core version < 2.0, so that code will continue to work with their simulator. Maybe we can just add warning section in above tutorial. Any thoughts? @DanPuzzuoli
from qiskit_experiments.framework import Options | ||
|
||
|
||
class ResonanceAnalysis(curve.CurveAnalysis): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep analysis itself because it doesn't depend on the pulse module. From the maintenance viewpoint, it's reasonable to remove unused code though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am not sure if there is something more we could do to preserve the old code... For now, I will personally just remember that the 0.8 release had all the pulse related code. I would rather remove it for now and pull it back later when it is useful. Otherwise, I think it will break over time as things change and it is not updated or it will add overhead as it is kept up to date without being used.
@@ -18,7 +18,6 @@ class unifies data access for various data fields. | |||
import warnings | |||
from qiskit.providers.models import PulseBackendConfiguration # pylint: disable=no-name-in-module | |||
from qiskit.providers import BackendV1, BackendV2 | |||
from qiskit.utils.deprecation import deprecate_func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this backend_data class? Since Qiskit will drop the V1 model, we don't need to maintain this abstraction layer. It would make sense to keep this for future models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely need to make changes to BackendData
so that it does not fail for deprecated Qiskit code references. I need to clean this PR up a little bit. The theme here is "remove the deprecated pulse and calibration experiments" and I think that is big enough for one PR. There is still some work to do to make sure the tests don't generate other deprecation warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the pulse related code was fairly extensive and is somewhat already not useful since IBM disabled pulse in the API already. For other Qiskit 2.0 changes like removing BackendV1, we might be able to provide a smoother transition between Qiskit 1 and Qiskit 2 without much effort (so we might not need to do a full removal right now).
error = 0.05 | ||
x_error = coherent_unitary_error(RXGate(error).to_matrix()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat :)
@@ -94,14 +95,10 @@ def _default_experiment_options(cls) -> Options: | |||
|
|||
Experiment Options: | |||
n_states (int): The number of states to discriminate. | |||
schedules (dict): A dictionary of the schedules for the gates in the experiment. Each key is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this experiment work? Qiskit doesn't have x12 gate in the builtin library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still works for 2 states. It would work for more states if a Qiskit backend provided instructions for exciting higher states. That doesn't seem too hard to imagine, so I thought we could keep this.
@@ -486,21 +484,6 @@ def _transpiled_circuits(self) -> List[QuantumCircuit]: | |||
for gate_1q in self.experiment_options.one_qubit_basis_gates: | |||
instructions.append((gate_1q, (q,))) | |||
|
|||
common_calibrations = defaultdict(dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also remove if-clause for BackendV2 above along this this code block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in d21b721
I guess with pinning qiskit to < 2.0 within Dynamics, will we also need to pin qiskit experiments to some version? Not sure how the internals would work without |
Right, that was my thought for the near term future. Since Qiskit 2.0 is coming out pretty soon, I just want to pull out anything that breaks with it for now. There is a plan to move pulse to dynamics and maybe we can put back some of the pulse related experiments and calibration helpers, or maybe it will make sense to move them to Dynamics as well to be closer to the pulse stuff. Since Qiskit 2.0 is removing things like |
We've actually decided to not move pulse to Dynamics, and instead just permanently pin the qiskit version for users doing |
Ah, okay, I hadn't seen that decision. So that aligns even more with what is done here -- just drop the pulse stuff and if you want it you can pin to version 0.8 of qiskit-experiments. I don't think it makes sense to try to maintain the code for a discontinued version of Qiskit. We could until Qiskit 1.x goes end of life but we are not planning on adding new features for the pulse stuff, so just pinning to qiskit-experiments should be the same effect. |
e9fcc07
to
14e078b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wshanks. Code change looks good to me. I wonder if we can release 1.0 to sync the release strategy with Qiskit core, as we are also introducing immediate breaking API change with this PR.
Hmm, which breaking API changes are you thinking of? Largely, this PR removes things that were deprecated in the previous release. There are some things removed that were not issuing deprecation warnings before like I think incrementing to 1.0 would be more a statement of stability than indicating breaking changes. I think our policy of trying to give one minor release of deprecation warning before removal is more in keeping with how much stability we want to aim for right now? |
Yes, I agree. But from your discussion above, I was just thinking it might be clearer for the users if we drop all of these components as a part of major release. The user can just pin with < 1.0 if they want pulse experiments. Also we don't actively develop the repo and code looks mature. |
Hmm, I am not sure about the status of development on the repo. The size of the team working on it is much smaller than it was and so the pace of change is also slower and it is more stable in that sense, but the code is still in use by some of the maintainers and I don't think they want to commit to more stability. For example if a feature breaks that is not used by the maintainers the response might be to remove it rather than fix it. |
This change prepares the package for Qiskit 2.0 by removing all features related to Qiskit Pulse which is removed in Qiskit 2.0 (see for example Qiskit#13872). It includes several related changes:
Rabi
andQubitSpectroscopy
for example. Additionally, analysis classes used only the removed experiments are also removed. These classes should have all emitted deprecation warnings in the previous release.Calibrations
andBasisGateLibrary
classes and the calibration experiment classes. These classes should have all emitted deprecation warnings in the previous release.PulseBackend
and dropqiskit-dynamics
as a dependency ofqiskit-experiments[extras]
. This is a last dependency on Qiskit Pulse that is not fully dropped yet because it was not previously deprecated.jupyter-execute
cells in the docs to use Qiskit Aer or mock backends instead of usingPulseBackend
.BackendData
andBackendTiming
likepulse_time
and the feature ofMultiStateDiscrimination
that allowed it to accept pulse schedules for exciting higher qubit states. Also, remove the ability to serialize and deserialize pulse schedule blocks withExperimentEncoder
andExperimentDecoder
.FineZXAmplitude
as it can not be easily used without a pulse calibration to implement an RZX rotation.ParallelExperiment
.ZZRamseyTestBackend
for theZZRamsey
tests and documentation that is similar toT2HahnBackend
to provide more consistency with the other experiment examples that were updated not to usePulseBackend
.Note that a few small pulse features might have been removed without a release cycle of deprecation warnings. Since the IBM Quantum service was the main use case of the Qiskit Pulse features and it has already stopped accepting jobs with Qiskit Pulse calibrations, it was deemed better to do a clean removal of all pulse features than to adhere strictly to the deprecation policy.
The changes here focus on the Qiskit Pulse related updates needed for compatibility with Qiskit 2.0. Some further follow up will also be necessary to address other incompatibilities with Qiskit 2.0.