Make InstructionScheduleMap support ParameterExpressions

[menehune23]

Summary

• Changes InstructionScheduleMap schedule generator params to allow ParameterExpressions in addition to numbers (Resolves InstructionScheduleMap parameters should be ParameterExpressions #3923).

[taalexander]

Looks good, just a couple minor changes recommended.

[taalexander]

Were we going to cast a ParameterExpression to a float in the ParameterizedSchedule if supplied?

[nkanazawa1989]

@menehune23 Thanks for the update. This is really great and I've been waiting this for long time.

• How do we bind actual parameters to the ParameterExpression?, i.e. synchronization with circuit parameter.

Pulse scheduler always pass *args. This cannot generate **kwargs due to its implementation.
https://github.com/Qiskit/qiskit-terra/blob/43c649018124de507d876cf3bfedc02abacbcce8/qiskit/scheduler/lowering.py#L87-L88

For example, is it possible to create an entry of Rx(theta) gate as shown in this paper?
https://arxiv.org/abs/2004.11205

We may want to add rx to basis gates:
https://github.com/Qiskit/qiskit-terra/blob/master/qiskit/circuit/library/standard_gates/rx.py#L23

Then scheduler pass theta to the instmap and it convert theta to amp of Gaussian. In this case theta may be also ParameterExpression with a bound float value.

• parameter ordering issue.

In ParameterizedSchedule the order of parameters is force sorted by key and thus function signature of generator is not remained.
https://github.com/Qiskit/qiskit-terra/blob/43c649018124de507d876cf3bfedc02abacbcce8/qiskit/pulse/schedule.py#L776

[menehune23]

Were we going to cast a ParameterExpression to a float in the ParameterizedSchedule if supplied?

@taalexander Per the discussion in #3923, my understanding was that for this issue, we'd cast to float just before passing to any callable schedule (i.e. functions and ParameterizedSchedules). Let me know if that's not what you had in mind.

I know that eventually, the ParameterizedSchedule will be replaced with a proper implementation that can take in ParameterExpressions directly (#3384). It was at that later time that I was planning to adjust the casting logic in the InstructionScheduleMap to only cast for functions, but leave them uncast (remaining as ParameterExpressions) for the new schedule type.

[menehune23]

@nkanazawa1989

• How do we bind actual parameters to the ParameterExpression?, i.e. synchronization with circuit parameter.

If you take a look at the test changes, you'll see how to bind a value to a ParameterExpression. In your case, inst.params can contain one or more ParameterExpressions (they must be bound, so that they can be cast to floats by the InstructionScheduleMap, as seen in this PR). Let me know if I understood your question correctly.

In ParameterizedSchedule the order of parameters is force sorted by key and thus function signature of generator is not remained.

I understand the problem, and I agree that it seems like a bug. That said, it looks like a pre-existing bug that should be filed separately.

[nkanazawa1989]

In your test case the parameter is bound before set to the generator function (this is the PR to add parameter object to schedule, so basically this test case is already enough). However we want to keep them as unbound parameter objects and bind a value before sending the program to the backend. Perhaps we can use parametric pulses because they can keep argument as parameters.
https://github.com/Qiskit/qiskit-terra/blob/04f20797eaa1a460e405adb8333508cd73bc448b/qiskit/pulse/library/parametric_pulses.py#L148-L149

I understand the problem, and I agree that it seems like a bug. That said, it looks like a pre-existing bug that should be filed separately.

This is not a bug, it's intentional. In the textualized CmdDef of U3 gate there are three parameters P1 P2 P3 associated with the shift phase instructions. However the order of instruction is not [P1, P2, P3] and we need to sort this to match with U3 gate definition to convert the gate into schedule without using **kwargs. We need to modify circuit parameter framework or write smart parser of CmdDef to address this.

I agree that those can be addressed in another PR.

[menehune23]

@nkanazawa1989 My apologies. I misunderstood where the binding was expected to happen. I'll look into this further when I have some time outside of work.

[menehune23]

@taalexander @nkanazawa1989 I'm realizing too that my original thoughts around this issue were based on replacing the param type with ParameterExpressions. Since we're actually just adding it as an additional type (adding to the Union type), I need to step backwards and rethink some of my original ideas. It is beginning to make more sense now though.

[menehune23]

@taalexander @nkanazawa1989 Now that I've had a chance to revisit this with better understanding, please take another fresh look. I summarized the changes in the release note, so please let me know if my understanding of when binds should occur is correct. Thanks!

[nkanazawa1989]

In this testcase dur_expr is fixed by dur_val defined in outer scope of generator (so parameter is kind of static). I understand that we have lack of framework that manages unbound parameters in the instruction schedule map, but here you intend in future this will be supported and dur_expr will be able to be updated dynamically? i.e.

inst_map.get('f', (0,), t=10)
inst_map.get('f', (0,), t=20)
inst_map.get('f', (0,), t=30)

[menehune23]

@nkanazawa1989 Ah yes, that should be possible. In fact, I’ll update the test case to demonstrate this, when I get a chance. Great thought! 👍

[menehune23]

@nkanazawa1989 Updated. Let me know your thoughts.

[nkanazawa1989]

Perhaps I misunderstood the purpose of this PR (in my example above it doesn't need to use ParameterExpression and we can just feed the parameter t into generator). Now I understand the intention of your test case. Anyways the code looks good now.