Fix pulse to signal converter

[to24toro]

Summary

This PR fixes the behavior of InstructionToSignals.get_signals.
Related with #140

Details and comments

Fix the behavior of get_signals when using Delay in qiskit-pulse.
Add a phase accumulation term when using shift or set frequency in qiskit-pulse.

[CLAassistant]

All committers have signed the CLA.

[DanPuzzuoli]

Thanks for these very important fixes.

I've added a couple of comments in the code, but aside from this, I think we need more tests to make sure that the behaviour you're putting in is actually being implemented. The test you've added is a good test, but it doesn't test the interaction between delays and SetFrequency/ShiftFrequency instructions. Can you add another test like test_delay, but where you do ShiftFrequency before and after the delay, to test the correct signal is being generated as in the Ramsi experiment? I'd also like to see tests with multiple ShiftFrequency and SetFrequency instructions.

I also think we should update the doc string for InstructionToSignals to be very clear about what rules it is using to generate samples. You could either do this here, or it can be done in a subsequent PR.

[DanPuzzuoli]

Can you change this to:

Suggested change

	phase_accumulations[chan] -= (
	(inst.frequency - (frequency_shifts[chan] + signals[chan].carrier_freq))
	* start_sample
	* self._dt
	)
	phase_accumulations[chan] += (
	((frequency_shifts[chan] + signals[chan].carrier_freq) - inst.frequency)
	* start_sample
	* self._dt
	)

I just find the negative slightly confusing in this case.

[to24toro]

I adapt this to phase_accumulations[chan] -= inst.frequency * start_sample * self._dt.
Do I also need to change the sign to plus ?

[DanPuzzuoli]

No that's okay - you can actually leave both as is - it was only a slight preference on my part, but doesn't really matter.

[to24toro]

Thank you for reviewing.
I added tests about the combination of ShiftFrequency, Set Frequency, and delay to confirm a phase accumulation. It may be a little bit too specific...

[to24toro]

I defined variables often used here. We can find other frequently used number in other tests. So We can define these variables often used at setUp method. I may refactor after merging this PR.

[DanPuzzuoli]

Nice changes to the tests - I think this pretty much covers everything.

I found the tests somewhat confusing to verify; the parentheses were a bit confusing and how the phase accumulation was calculated was not explicit. I've just opened a PR to your branch that changes the tests to how I'd like them to look.

If you can take a look at that and merge it if it looks fine, then the only things left to do are:

• It seems you need to sign the CLA still.
• Make sure this PR passes the linter.

[to24toro]

Thank you for improving my codes.
Definitely, predefining phase_accumulation and freq_shift is more readable.

I fixed the author committing to this PR because I used the wrong one.

[DanPuzzuoli]

I just made some final documentation changes, and it looks good to me! Will merge once tests pass.