Implement soft delay extension

[btasdelen]

Work in progress. Effort to implement soft delay extension for file format 1.5.0 compatibility.

TODO:

• [x] Add soft_delay handling to addBlock().
• [x] Implement apply_soft_delay().
• [x] Add soft_delay handling to write().
• [x] Add soft_delay handling to read().
• [x] Implement get_default_soft_delay_values()
• [ ] Update check_timing().
• [x] Implement register_soft_delay_event()
• [ ] Add tests for soft delay.
• [ ] Add example sequence.

Challenges/Concerns:

• [ ] Adding soft delays seemed to change some extension ID's, causing sequence tests to fail as the .seq files do not match. Invesigate/fix.

[github-actions[bot]]

Coverage Report

File	Stmts	Miss	Cover	Missing
/home/runner/.local/lib/python3.12/site-packages/pypulseq
add_gradients.py	123	51	59%	44, 52, 58, 61, 75–86, 92, 120–123, 130–131, 150, 157, 162–241
add_ramps.py	36	36	0%	1–89
align.py	35	4	89%	41, 45, 69, 73
calc_duration.py	25	1	96%	37
calc_ramp.py	218	214	2%	45–353
calc_rf_bandwidth.py	27	20	26%	37–59, 63–67
check_timing.py	96	29	70%	78, 82, 107, 180, 199, 232, 239, 249–293
compress_shape.py	30	1	97%	28
convert.py	40	8	80%	42, 48, 66, 72–73, 82, 88–89
event_lib.py	96	14	85%	6–9, 48–51, 70–71, 205–210
make_adc.py	92	13	86%	63, 72–76, 79, 128, 131, 135, 141, 145, 184, 186, 188, 196
make_adiabatic_pulse.py	129	39	70%	196–200, 217–221, 229–230, 253, 259, 328–347, 451–460, 498–506
make_arbitrary_grad.py	37	7	81%	68, 71, 74, 77, 81, 83, 103
make_arbitrary_rf.py	66	55	17%	83–160
make_block_pulse.py	46	3	93%	112–116, 119
make_delay.py	9	1	89%	27
make_digital_output_pulse.py	16	2	88%	39, 47
make_extended_trapezoid.py	56	12	79%	67, 70, 76, 82, 85, 88, 91, 94, 116, 134, 136, 139
make_extended_trapezoid_area.py	93	3	97%	52, 227, 230
make_gauss_pulse.py	69	20	71%	127–131, 134–158, 165, 168
make_label.py	22	4	82%	64, 66, 68, 75
make_sigpy_pulse.py	116	31	73%	12–13, 112, 115, 119, 154, 157–161, 165, 168–169, 172–173, 188, 195, 200, 212, 215, 240–250, 264, 267, 297–307
make_sinc_pulse.py	68	10	85%	94, 100, 127–131, 135, 138–139, 142–143, 165
make_soft_delay.py	25	2	92%	102, 120
make_trapezoid.py	111	7	94%	177, 190, 196, 214, 232, 237, 255
make_trigger.py	16	2	88%	44, 52
opts.py	66	9	86%	78, 83, 102, 142, 166–170
points_to_waveform.py	9	1	89%	27
rotate.py	69	14	80%	15, 55, 66–69, 85–90, 112, 119–120
scale_grad.py	14	4	71%	28–30, 33
sigpy_pulse_opts.py	26	7	73%	34–41
split_gradient.py	39	31	21%	46–103
split_gradient_at.py	70	27	61%	63–90, 110, 114, 118–120, 154–156
traj_to_grad.py	13	9	31%	26–40
/home/runner/.local/lib/python3.12/site-packages/pypulseq/SAR
SAR_calc.py	113	98	13%	33–40, 55–62, 89–108, 129–132, 168–212, 242–246, 264–306
/home/runner/.local/lib/python3.12/site-packages/pypulseq/Sequence
block.py	405	37	91%	63, 66, 74, 80, 95, 103, 109, 120, 123, 126, 134, 139, 148, 159, 167, 207, 209, 213, 225, 274, 278, 294, 334–337, 366–367, 433, 439, 472, 541, 577, 583, 610, 648, 727
calc_grad_spectrum.py	81	76	6%	68–190
calc_pns.py	40	31	22%	45–96
ext_test_report.py	144	12	92%	23, 61, 138, 149–150, 237–243
install.py	75	42	44%	31, 52, 69, 71, 112–131, 148, 181–184, 200–212, 254–278
parula.py	4	2	50%	19–86
read_seq.py	319	68	79%	42–43, 90, 93, 105, 110, 116, 123, 132, 141, 146, 149, 157–159, 202, 207, 215–264, 294–297, 312–313, 342–359, 422, 425, 460, 468, 542, 584–588
sequence.py	792	241	70%	11–14, 104–114, 135–148, 195, 260–263, 310, 337, 354, 402, 430, 457–462, 499, 515, 606, 628, 669–672, 726, 764, 775–776, 782, 793, 799, 801, 809, 842–850, 871–893, 936, 938, 941, 967–968, 971–974, 1010–1020, 1029–1031, 1075–1087, 1102–1103, 1119–1137, 1170–1171, 1197, 1203, 1206, 1209, 1246, 1367–1380, 1403, 1431, 1453–1455, 1476, 1539, 1547, 1614, 1625–1638, 1650–1661, 1707–1708, 1717–1735, 1759, 1789–1797, 1829–1939, 1975, 1989–1999, 2003, 2014
write_seq.py	354	176	50%	42, 66, 69–76, 303–526
/home/runner/.local/lib/python3.12/site-packages/pypulseq/utils
cumsum.py	14	1	93%	17
safe_pns_prediction.py	126	113	10%	50–87, 102–189, 197–214, 222, 244–250, 279–286, 310–336, 344–383, 396–411, 415
tracing.py	16	6	62%	33–34, 42, 54–55, 75
/home/runner/.local/lib/python3.12/site-packages/pypulseq/utils/siemens
asc_to_hw.py	58	53	9%	21–28, 48–106
readasc.py	48	45	6%	25–100
TOTAL	4696	1692	64%

Tests	Skipped	Failures	Errors	Time
1335	21 :zzz:	0 :x:	0 :fire:	3m 28s :stopwatch:

[FrankZijlstra]

Regarding the failing sequence tests, you can set the environment variable SAVE_EXPECTED=1 and run pytest to locally rewrite the .seq files. This will automatically pass the write tests, but then allows you to do a diff with the previous .seq files (e.g. git diff, or make a copy of the .seq files first). Then if you manually verify that the change of IDs is expected, commit the new .seq files. I know manually checking this is a bit of a pain, but I think aiming for a exact match of the .seq files is a good standard, better safe than sorry.

[btasdelen]

I think this is ready for testing, but there are several things that may require polishing.

1. I did not add any tests because I could not come up with reasonable unit tests for this.
2. I did not see specific changes to plot() or checkTiming() functions in Matlab version, so I couldn't figure out how/if they handle the plotting of soft delays.
3. This required allowing float input to add_block() function to mimic the behavior in Matlab, which to my understanding is to mainly set required_duration variable. I feel like this could be a separate function parameter with a default value None. My only concern with that is this variable only seems to be used for soft delay anyways, so why is this not a member of soft delay object? How much to diverge from Matlab?
4. Should we merge this to main, or a separate 1.5.0 branch? With the example scripts, without using any soft delays, there is no change to .seq files, so I hope it is safe to merge to main.

[fzimmermann89]

regarding check timings: for it to make sense, it would need to get the information about the values that should be used during the check, correct? maybe a dict of values? and check_timings should warn that the timings are only checked for this particular setting of delay values, other settings in the scanner UI can break.

or would the expected workflow be save seq, apply_soft_delay for all delays, check_timings ? then check_timings should complain if there are any "unapplied" soft delays. or am i misunderstanding how soft delays are supposed to work?

[fzimmermann89]

PS: I am stricly in favor of diverting from matlab to make parameters and functions more explicit and pythonic.

[FrankZijlstra]

General comment: It's probably a good idea to add write_gre_label_softdelay to the example sequence tests (test_sequence.py line 215).

[btasdelen]

@FrankZijlstra @fzimmermann89 Many thanks for the comments. It looks like #288 takes this PR and has many changes on top of this. I am not sure how to proceed with this without duplicating effort and creating some conflicts.

It looks like there are some changes to soft delay part, but most changes on #288 are for other features. If @mcencini wants to adress these feedback on there I can close this PR.

[mcencini]

@btasdelen apologies, I did not mean to create confusion. The only changes in the soft-delay-part are some bug fixes and a simple test case that I wanted to add to this PR (see btasdelen/pypulseq#2). The idea was to develop the remaining v1.5.0 features mentioned in #258, but wait until this PR was merged.

That said, I am happy to help polishing this. If you think you don't need to have soft delay extension in 1.4.x, I can address it in #288. The only issue is that the v1.5.x changes are quite a lot and I unfortunately I do not see an easy way to split it in multiple smaller PR for easy review, except for soft delay which is quite independent from the rest. Suggestions are welcome, either in #288 or #258 threads! :D

[btasdelen]

@mcencini No worries. That is why I started with soft delays, it was pretty much independent of the rest of the stuff.

I can try cherry-picking your relevant commits. If I make changes and merge this, I believe you will have to deal with some conflicts when you update your branch.

I am fine either way. My opinion is, regardless who deals with the rest, it is better to divide things into smaller PRs, even if the individual PRs are not complete v1.5.0 compatible.

[btasdelen]

@fzimmermann89 @FrankZijlstra I had a crack at making numID optional, and making soft_delay_hints a normal dictionary to always refer soft delay's by their hints. I realized it is done to keep EventLibrary's data integer only. We don't have that restriction in Python, so I save the hint as string now. soft_delay_hints is a normal dict now, and its only purpose is to make sure numID -> hint mapping is unique.

Another way to approach this is to make use of the soft_delay_hints dictionary, and just not store hint in EventLibrary, instead we can refer to delays by their numID. For this, though, I believe I need to make soft_delay_hints a bijective dictionary again to fetch numID when user provided hint.

Also, I did not remove numID input but made it optional. Let me know if this looks reasonable.

[btasdelen]

@fzimmermann89 @FrankZijlstra I think this is the only thing left before the v1.5.0 release. Let me know if you have any further comments. I know it is not perfect, but one option to be just merge this and see if folks use it and have any bug reports on it. Since this is pretty much decoupled from the rest of the code, it should be okay.

[FrankZijlstra]

I think it would be okay to merge it with a few minor changes:

• Add write_gre_label_softdelay to the example sequence tests in test_sequence. This checks basic things like writing and reading the sequence, which is good to test the soft delay extension blocks in the sequence file.
• Fix get_default_soft_delay_values so it does not contain the error report stuff. I do not fully understand the checks being done, but it seems to me it should be in check_timing (or maybe it can be checked immediately when the block gets added?). These can be left as placeholders wherever it makes most sense to put these checks. Note that check_timing automatically gets called before writing a sequence, so it is the most safe for any sequence checks that would breaks things on the scanner.

[btasdelen]

@FrankZijlstra

• Done.
• I moved the remaining timing check into check_timing and removed the error_report stuff. I kept the other checks as they are not timing checks, but I can remove them if you want to. I can even remove the whole get_default_soft_delay_values function if it bothers you, because I don't really understand its purpose or if it works properly...

[btasdelen]

@schuenke I reviewed the changes. I think this PR looks pretty good at the moment.

I agree soft_delay_simple_example is useful. I wouldn't worry about it looks different from other examples.

I am fine with this being merged if there are no objections.

[schuenke]

I added the soft delays to sequence.plot() now. I decided to add it to the RF phase subplot (this is usually pretty empty), but we could also add it to all subplots because soft delay blocks have to be empty anyway. For the duration I use the default duration. This is how it looks for realistic cases with a long and a very short default duration:

What's your opinion on this @btasdelen

Edit: here is the code to generate the plot

import pypulseq as pp

seq = pp.Sequence()
rf, gz, gzr = pp.make_sinc_pulse(flip_angle=0.5, duration=2.56e-3, slice_thickness=1e-3, return_gz=True)
seq.add_block(rf, gz)
seq.add_block(gzr)
seq.add_block(pp.make_soft_delay('TE', default_duration=1e-6))
gx = pp.make_trapezoid('x', flat_area=100, flat_time=5e-3)
adc = pp.make_adc(num_samples=128, duration=5e-3, delay=gx.rise_time)
seq.add_block(gx, adc)

seq.plot()

[btasdelen]

@schuenke How about this:

Shade is on all axes, but annotation is on Phase only. Is just using the delay hint to annotate too ambiguous?

Alternative annotation: SD: "TE"