waveforms_and_times append RF fixes

[btasdelen]

Waveforms and times with append RF parameter is not functioning properly, due to improper casts and some missing logics. Also, it was omitting the phase and frequency offsets.

This is a hacky fix and open to discussion. Upstream MATLAB implementation uses cell arrays for storing the time/waveform pairs, which is more flexible than the numpy arrays and nested lists used here. As the RF waveform is complex, usage of np array forces the associated time points to be complex as well, which is a bit awkward and inefficient.

I have two proposals for a better solution:

1) We can switch to dictionaries, and only the time points and waveform would be np arrays. Example usage:

waves['gx']['t'] # For time points
waves['gx']['s'] # For waveform
waves['rf']['s'] # For waveform (only this one is complex)

I think this will be a more flexible, verbose and Pythonic way of doing this, but it slightly deviates from upstream.

2) We can split RF into phase and magnitude so that the RF list will be like:

waves[3][0] # RF time
waves[3][1] # RF magnitude
waves[3][2] # RF phase

This will deviate less from the upstream, but adds even more complication to the output of this function, which I think feels confusing and hard-coded right now.

[sravan953]

Thank you! My work on PyPulseq 1.4.0 so far was referencing Pulseq 1.4.0 as per the commit on Dec 21, 2021. We are already 1y since that commit. So once again, there are lots of changes to catch up on. ;)

[sravan953]

Regarding your proposal, are you suggesting we migrate the wave_data variable from being an object np.array to a dict?

[btasdelen]

It is a list of np.array (2xN) now. What I suggest is, it can be a list of list (2 elements) of np.array (1xN), or a dict of dict (or list) of numpy array (1xN).

The reason I suggest dicts is more towards user-friendliness. Instead of assuming the order of gradients and rf, user can call by name. But I understand that it also makes pypulseq deviate more from the Pulseq's interface.

[sravan953]

I agree with you, and I favor dict, that's a good idea. To clarify, and for record keeping, as per your suggestion, we would migrate to wave_data being a dict of lists.

[btasdelen]

Great! I can implement and open a PR for it when I have the time.