Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.25k stars 2.37k forks source link

Fix issue with assigning parameters to pulse gate definitions #5524

Open lcapelluto opened 3 years ago

lcapelluto commented 3 years ago

Information

What is the current behavior?

If a schedule is added as a calibration, and has a parameter which isn't in the gate, it cannot be assigned via circ.assign_parameter.

Steps to reproduce the problem

from qiskit import QuantumCircuit, pulse
from qiskit.circuit import Parameter

qc = QuantumCircuit(1)
qc.x(0)

alpha = Parameter('alpha')

with pulse.build() as sched:
    pulse.shift_phase(alpha, pulse.DriveChannel(0))

qc.add_calibration('x', [0], sched, [alpha])

qc.assign_parameters({alpha: 0.5})

What is the expected behavior?

alpha in the example schedule above, should be assigned to 0.5. This can be checked via qc.calibrations.

Suggested solutions

assign_parameters calls _assign_parameters, which in turn calls _assign_calibration_parameters. It's possible the last of those does what we want, but the method errors before it can get there, since it sees that the input parameter is not in self._parameter_table. The best solution may be to add schedule.parameters to the _parameter_table when add_calibration is called. The alternative approach would be to modify the error condition in assign_parameters to not error right away -- but it might not be that straightforward to raise the proper error when the schedule also doesn't have that parameter.

Drinion commented 3 years ago

Hi! May I have a shot at this?

lcapelluto commented 3 years ago

Absolutely!

Drinion commented 3 years ago

Ok, I've now added self._update_parameter_table(schedule.parameters) to add_calibrations() and it produces the following error:

AttributeError                            Traceback (most recent call last)
<ipython-input> in <module>
     10     pulse.shift_phase(alpha, pulse.DriveChannel(0))
     11 
---> 12 qc.add_calibration('x', [0], sched, [alpha])
     13 
     14 qc.assign_parameters({alpha: 0.5})

~/qiskit-terra/qiskit/circuit/quantumcircuit.py in add_calibration(self, gate, qubits, schedule, params)
   2538         else:
   2539             self._calibrations[gate][(tuple(qubits), tuple(params or []))] = schedule
-> 2540             self._update_parameter_table(schedule.parameters)
   2541 
   2542     # Functions only for scheduled circuits

~/qiskit-terra/qiskit/circuit/quantumcircuit.py in _update_parameter_table(self, instruction)
    942 
    943     def _update_parameter_table(self, instruction):
--> 944         for param_index, param in enumerate(instruction.params):
    945             if isinstance(param, ParameterExpression):
    946                 current_parameters = self._parameter_table

AttributeError: 'set' object has no attribute 'params'

The issue stems from the way I'm trying to add the schedule.parameters to the _parameters_table. Am I using the wrong method?

iamsantanubanerjee commented 3 years ago

@Drinion I've been looking at the issue for some time now and from my understanding (and also from what @lcapelluto has confirmed) schedule.parameters and _parameter_table has two different data structures.

Drinion commented 3 years ago

@iamsantanubanerjee Thanks! Yes, I suggest that we adjust the error message instead of trying to modify add_calibration(). This will be quite elaborate however.

paolob67 commented 3 years ago

Hey @Drinion, @lcapelluto I suggest changing add_calibration() like this.

    def add_calibration(self, gate, qubits, schedule, params=None):
        """Register a low-level, custom pulse definition for the given gate.

        Args:
            gate (Union[Gate, str]): Gate information.
            qubits (Union[int, Tuple[int]]): List of qubits to be measured.
            schedule (Schedule): Schedule information.
            params (Optional[List[Union[float, Parameter]]]): A list of parameters.

        Raises:
            Exception: if the gate is of type string and params is None.
        """
        if isinstance(gate, Gate):
            self._calibrations[gate.name][(tuple(qubits), tuple(gate.params))] = schedule
            _name = gate.name
        else:
            self._calibrations[gate][(tuple(qubits), tuple(params or []))] = schedule
            _name = gate

        # 5524 add parameters to the _parameter_table
        for instruction, qargs, cargs in self._data:
            if instruction.name == _name:
                instruction.params = params
                self._update_parameter_table(instruction)

A quick test but, probably, needs more:

from qiskit import QuantumCircuit, pulse
from qiskit.circuit import Parameter

qc = QuantumCircuit(1)
qc.x(0)

alpha = Parameter('alpha')

with pulse.build() as sched:
    pulse.shift_phase(alpha, pulse.DriveChannel(0))

qc.add_calibration('x', [0], sched, [alpha])

qc.assign_parameters({alpha: 0.5})

qc.draw()
     ┌──────────┐
q_0: ┤ X(alpha) ├
     └──────────┘

I hope it helps.

Paolo

Drinion commented 3 years ago

@paolob67 Perfect! Thanks, I'll test your solution and set up a merge request.

paolob67 commented 3 years ago

You might also want to handle the case when the instruction is not found and raise some kind of error. Also, the params could already be set, therefore you might want to append instead of replacing.

Drinion commented 3 years ago

@lcapelluto Sorry, my merge request pipeline tests failed and I don't have the time right now to continue with this issue, so I've unassigned myself.

fs1132429 commented 3 years ago

I would like to work on this issue if its ok. I am new to contributing.

lcapelluto commented 3 years ago

Okay! Start from here: https://github.com/Qiskit/qiskit-terra/pull/5568 If you need tips on how to do that, let me know.

fs1132429 commented 3 years ago

@lcapelluto I went through #5568 . Not really sure how to proceed. Can you guide me? I am new to contributing.

lcapelluto commented 3 years ago

Hi @fs1132429 , this one is a little bit tricky -- there might be another good first issue that isn't already partway done that you can try instead. That way, you can get the hang of contributing to open source first (and using github, if you haven't before) and then round back on this one later :)