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

2 Qubit Pulse Backend #971

Open
wants to merge 144 commits into
base: main
Choose a base branch
from
Open

Conversation

rupeshknn
Copy link
Contributor

@rupeshknn rupeshknn commented Nov 10, 2022

Summary

Further to #925, in this PR, we add a 2-qubit pulse backed for parallel experiments and multi-qubit gates.
This PR is part of the Qiskit Advocate Mentorship Program (QAMP-Fall22).

Details and comments

Co-Authors:
JeongWon Kim @bicycle315
Daniel Egger @eggerdj

  • 2 qubit parallel backend (decoupled)
  • 2 qubit backend with multi-qubit gates
  • Corresponding tests

Copy link
Collaborator

@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.

Thanks @rbuckland @rupeshknn for implementing two qubit backend. This is necessary for unittest of future two qubit PRs. I think current PulseBackend is not capable of implementing two qubit architecture, and you need a major refactoring of its constructor. Please check my review comments below.


def __init__(
self,
qubit_frequency: float = 5e9,
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 Jun 12, 2023

Choose a reason for hiding this comment

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

This needs refactoring. If you give the save frequency to both qubits this situation is so called Type1 collision and you cannot control qubits under the collision. What I was trying to implement was

    @deprecate_arg(
        name="static_hamiltonian",
        since="0.6",
        package_name="qiskit-experiments",
        pending=True,
    )
    @deprecate_arg(
        name="hamiltonian_operators",
        since="0.6",
        package_name="qiskit-experiments",
        pending=True,
    )
    @deprecate_arg(
        name="static_dissipators",
        since="0.6",
        package_name="qiskit-experiments",
        pending=True,
    )
    def __init__(
        self,
        static_hamiltonian: Optional[np.ndarray] = None,
        hamiltonian_operators: Optional[np.ndarray] = None,
        static_dissipators: Optional[np.ndarray] = None,
        solver: Optional = None,
        dt: float = 0.1 * 1e-9,
        solver_method="RK23",
        seed: int = 0,
        **kwargs,
    ):

Instead of providing Hamiltonian, the PulseBackend class should take solver itself. This is because you also need to provide channel name mapping for control channels.

Then you can do

        if static_hamiltonian is not None and hamiltonian_operators is not None:
            # TODO deprecate soon
            solver = Solver(
                static_hamiltonian=static_hamiltonian,
                hamiltonian_operators=hamiltonian_operators,
                static_dissipators=static_dissipators,
                **kwargs,
            )
        self._static_hamiltonian = static_hamiltonian
        self._hamiltonian_operators = hamiltonian_operators
        self._static_dissipators = static_dissipators
        self.solver = solver

It currently implements a condition for initial state dimension based on static_dissipators, but this can be also done with typecheck of solver.model, when static_dissipators is not explicitly provided.

        if isinstance(self.solver.model, LindbladModel):
            self.y_0 = np.eye(self.model_dim**2)
            self.ground_state = np.array([1.0] + [0.0] * (self.model_dim**2 - 1))
        else:
            self.y_0 = np.eye(self.model_dim)
            self.ground_state = np.array([1.0] + [0.0] * (self.model_dim - 1))

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also against to having user provided frequency and anharmonicity. Because this changes the Hamiltonian, you need to dynamically update the hard-coded calibrations. For example, currently DRAG pulse is not considered in the calibration but changing anharmonicity may change DRAG coefficient. The missing DRAG term may impact the unittest of the experiments requiring precise state preparation or echo, such as error amplification.

I prefer to have DRAG pulses and remove frequencies and anharmonicities from the constructor to preserve the hard-coded calibrations.

self._target = Target(
num_qubits=2,
qubit_properties=[
QubitProperties(frequency=qubit_frequency),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not precise in case of 2Q. Since qubits are dressed by weak J coupling, you need to consider the coupling term under RWA and diagonalize the static Hamiltonian to obtain the eigenfrequencies.

}

@staticmethod
def rz_gate(qubits: List[int], theta: float) -> np.ndarray:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rz must consider the frame synchronization with control (cross resonance) channels in case of 2Q. We don't provide qutrit control so this is not required in minimal setup.


self.subsystem_dims = (3, 3)

self._defaults = PulseDefaults.from_dict(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering why _defaults is needed. This is not the scope of this PR but this is no longer the attribute of V2Backend. You should not have dependency on it, except for getting qubit_freq_est and meas_freq_est (currently these information are provided only by defaults).

"buffer": 0,
"pulse_library": [],
"cmd_def": [
self.pulse_command(name="x", qubit=0, amp=(0.5 + 0j) / self.rabi_rate_01[0]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

These calibration should go to QubitProperties.calibration

Copy link
Contributor Author

@rupeshknn rupeshknn Jun 14, 2023

Choose a reason for hiding this comment

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

I couldn't find a reference for QubitProperties.calibration can you point me to where this class is located?
the one in qiskit.providers.backend.QubitProperties does not have a calibration attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you meant to say InstructionProperties.calibration. I plan to use it something like:

pulseinst = PulseQobjInstruction(
    name="parametric_pulse",
    t0=0,
    ch=f"d{0}",
    label=f"Xp_d{0}",
    pulse_shape="drag",
    parameters={
        "amp": 0.02,
        "beta": 5,
        "duration": 160,
        "sigma": 40,
    },
)

pqd = PulseQobjDef(name='x')
pqd.define([pulseinst])
x_props = {
            (0,): InstructionProperties(duration=160e-10, error=0, calibration=pqd),
        }
self._target.add_instruction(XGate(), x_props)

let me know if you have other suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

PulseQobjInstruction is not recommended unless you want to save it in JSON format in human readable way. This is a legacy representation of pulse sequence for wire format, and not friendly to manually write (it becomes quickly complicated if you try to write two qubit gate schedule). Instead you can directly create ScheduleBlock with the builder pattern.

)

measure_props = {
(0,): InstructionProperties(duration=0, error=0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can give duration since you provided the default calibration.

@nkanazawa1989
Copy link
Collaborator

For your information, you can follow this Qiskit Dynamics tutorial for Simulating backends at the pulse-level with DynamicsBackend. Because several extra arguments are needed to build the Solver instance for a two qubit device, it's much cleaner to allow PulseBackend to take a solver instance in its constructor, rather than adding more arguments. This will also make the PulseBackend robust to future API change of dynamics Solver instance.

@rupeshknn
Copy link
Contributor Author

Thank you for your review @nkanazawa1989. I'll go through them and make changes accordingly.

As for using DynamicsBackend, there was an initial meeting with the qiskit-dynamics team and at that point, we had concluded that it was better to have these as independent efforts, primarily to keep things simple and efficient for the tests rather than having something more general.
That said, I will check if it is now possible to incorporate DynamicsBackend in PulseBackend without disrupting too much.

@nkanazawa1989
Copy link
Collaborator

nkanazawa1989 commented Jun 13, 2023

Yeah I know the PulseBackend implements different approach for circuit simulation, so I don't suggest replacing it with Dynamics Backend (though we can make it a subclass of dynamics and just override the run method to reuse rest of some nice features). The dynamics tutorial is about setup of Solver instance.

@rbuckland
Copy link
Contributor

Thanks @rbuckland for implementing two qubit backend. This is necessary for unittest of future two qubit PRs. I think current PulseBackend is not capable of implementing two qubit architecture, and you need a major refactoring of its constructor. Please check my review comments below.

:-) that puzzled me - was not I that created the PR.
I can see that the correct author, @rupeshknn has picked up review.

@nkanazawa1989
Copy link
Collaborator

@rbuckland Oh, sorry about bothering you, and thanks for the message. I updated my review comment.

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2023

CLA assistant check
All committers have signed the CLA.

@Naohnakazawa
Copy link
Contributor

Hello, @rupeshknn. Are you still working on this issue?

@rupeshknn
Copy link
Contributor Author

Hi @nkanazawa1989 I've gotten busy with grad school. I don't think I'll be able to get back to this PR anytime soon. It would be nice if someone could take over. Sorry and thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants