Were you able to find out what caused the problem you are describing?

Were you able to find out what caused the problem you are describing?

Not specifically. I saw the same thing happen when I did the bit shift for the acquisition gate signals in the wrong direction which essentially caused the gate to be high continuously, so whatever the bug was in the old code it must've been causing that gate signal to be high.

The other time I saw incorrect acquisition gating was with the fix I pushed previously which was when trying to replicate the control signals on the extra GPIO cards when those wernt installed. It would cause the TX card (process?) to crash but it would have jitter on the output of the GPIO pins on the card so the Rx card would just randomly acquire a few samples here and there. Very different behaviour, so I think it was related to the gate signal, not something else.

Thank you for the PR @Toreil! I added some comments to the code, have you been able to verify the sequence calculation updates? Maybe a direct comparison of the unrolled sequence waveforms from the old implementation vs. the new implementation is a good test.

Yeah I did a direct comparison both on the bench and with imaging experiments and performance was identical. As far as I can tell merge is ready to go (I don't understand the failed tests, I ran them locally and there was no problem). I also tested seperately on Python 3.12 and confirm it's all working so the dependencies can be updated to reflect that.

Thank you for the PR @Toreil! I added some comments to the code, have you been able to verify the sequence calculation updates? Maybe a direct comparison of the unrolled sequence waveforms from the old implementation vs. the new implementation is a good test.

Yeah I did a direct comparison both on the bench and with imaging experiments and performance was identical. As far as I can tell merge is ready to go (I don't understand the failed tests, I ran them locally and there was no problem). I also tested seperately on Python 3.12 and confirm it's all working so the dependencies can be updated to reflect that.

That is great, but I think it would be good to have an objective measure. For example. by unrolling a reference sequence with the current and the updated version of the sequence provider and reporting the sum of absolute difference between the resulting arrays (should be 0). I know this causes some extra work, but we have had some bugs in the past because we did not check changes carefully. I will have a look at the pytest error.
Regarding the static tests, mypy throws some errors. You should be able to see them by running mypy src/ in the project root. The CI which executes the test always uses a fresh install of the package. It could be, that there is a version missmatch. Let me know, if you need further support with this.

That is great, but I think it would be good to have an objective measure. For example. by unrolling a reference sequence with the current and the updated version of the sequence provider and reporting the sum of absolute difference between the resulting arrays (should be 0). I know this causes some extra work, but we have had some bugs in the past because we did not check changes carefully. I will have a look at the pytest error. Regarding the static tests, mypy throws some errors. You should be able to see them by running mypy src/ in the project root. The CI which executes the test always uses a fresh install of the package. It could be, that there is a version missmatch. Let me know, if you need further support with this.

I ran a set of comparisons using the example sequences in the examples folder since they cover all basic functions of Pulseq. There are/were small differences but they're all attributable to rounding of the number of samples (which for shaped RF pulses has an outsized visual effect due to using resample which depends on the number of samples). The fast-unroll uses round to cast to int, whereas the main branch casts to int which always rounds down. Typically this manifests as a 20 us delay being 400 samples on the fast-unroll and 399 on the main branch of the sequence unroll for instance. I propose that since the small changes are due to round being used and round gives a more accurate number of samples the small differences are not detrimental (perhaps even an improvement) and this branch is ready to merge.

That is great, but I think it would be good to have an objective measure. For example. by unrolling a reference sequence with the current and the updated version of the sequence provider and reporting the sum of absolute difference between the resulting arrays (should be 0). I know this causes some extra work, but we have had some bugs in the past because we did not check changes carefully. I will have a look at the pytest error. Regarding the static tests, mypy throws some errors. You should be able to see them by running mypy src/ in the project root. The CI which executes the test always uses a fresh install of the package. It could be, that there is a version missmatch. Let me know, if you need further support with this.

I ran a set of comparisons using the example sequences in the examples folder since they cover all basic functions of Pulseq. There are/were small differences but they're all attributable to rounding of the number of samples (which for shaped RF pulses has an outsized visual effect due to using resample which depends on the number of samples). The fast-unroll uses round to cast to int, whereas the main branch casts to int which always rounds down. Typically this manifests as a 20 us delay being 400 samples on the fast-unroll and 399 on the main branch of the sequence unroll for instance. I propose that since the small changes are due to round being used and round gives a more accurate number of samples the small differences are not detrimental (perhaps even an improvement) and this branch is ready to merge.

Could you share some of these results then? Maybe just post some magnitude/phase images of the different states here for documentation, that would be great!
I'll have a look at the remaining errors in pytest and the static tests.

Hi all,

Do not have the full story here. The conversation triggered me: if int or rounding errors causes some phase issues that I have not been able to solve at issue #66 , which was apparent at 3T experiments as well. Have you tested if Decimal works (worse calculation performance compared to round or int but maybe the most stable for imaging) ? I implemented in Rx device as follows:

# Calculate gate duration
gate_length = Decimal(str(timestamp_1)) - Decimal(str(timestamp_0))

# Calculate the number of adc gate sample points (per channel)
gate_sample = int(round(gate_length * (Decimal(str(self.sample_rate)) * Decimal("1e6"))))   

This fixes small rounding errors for delay and phase instabilities as mentioned above.

Hi all,

Do not have the full story here. The conversation triggered me: if int or rounding errors causes some phase issues that I have not been able to solve at issue #66 , which was apparent at 3T experiments as well. Have you tested if Decimal works (worse calculation performance compared to round or int but maybe the most stable for imaging) ? I implemented in Rx device as follows:

# Calculate gate duration
gate_length = Decimal(str(timestamp_1)) - Decimal(str(timestamp_0))

# Calculate the number of adc gate sample points (per channel)
gate_sample = int(round(gate_length * (Decimal(str(self.sample_rate)) * Decimal("1e6"))))   

This fixes small rounding errors for delay and phase instabilities as mentioned above.

Hi Berk,

thanks for your input on this, I think this is more about convention when unrolling the sequence.
Instead of truncating the calculated number of sample points by int(...), the use of round yields an extra sample point for some cases. However, because the waveforms are interpolated anyway, this does not matter. This is about a one data point on a 50 ns sampling grid.

Thank you for the readme update! Please use a seperate PR (even for such small changes) in the future.

Comparison of gradient waveforms with the latest fix showing (essentially) identical waveforms, small difference are caused by different rounding of gradient event durations.