Conversation
- Added comparison methods for `Sequence` and `Extension` types to facilitate equality checks. - Updated `read_seq` and `get_block` functions to include a `simplify_shapes` keyword argument for improved shape handling. - New `write_seq` function and new structures for managing Pulseq assets. - Introduced utility functions for quantizing time and managing Pulseq export tests, ensuring compatibility. - Added tests for round-trip sequence reading and writing to validate the new functionality.
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (87.53%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #718 +/- ##
==========================================
- Coverage 91.03% 87.53% -3.51%
==========================================
Files 61 57 -4
Lines 3357 3729 +372
==========================================
+ Hits 3056 3264 +208
- Misses 301 465 +164
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
KomaMRI Benchmarks
Details
| Benchmark suite | Current: 17d943d | Previous: 43a3ab1 | Ratio |
|---|---|---|---|
MRI Lab/Bloch/CPU/2 thread(s) |
333149869 ns |
335386708 ns |
0.99 |
MRI Lab/Bloch/CPU/4 thread(s) |
275996251.5 ns |
274571608 ns |
1.01 |
MRI Lab/Bloch/CPU/8 thread(s) |
258167686 ns |
247370806 ns |
1.04 |
MRI Lab/Bloch/CPU/1 thread(s) |
557250045 ns |
555875715 ns |
1.00 |
MRI Lab/Bloch/GPU/CUDA |
20560080 ns |
20834632 ns |
0.99 |
MRI Lab/Bloch/GPU/oneAPI |
73159990 ns |
73526556 ns |
1.00 |
MRI Lab/Bloch/GPU/Metal |
97828958 ns |
93171166 ns |
1.05 |
MRI Lab/Bloch/GPU/AMDGPU |
23782296.5 ns |
23841938 ns |
1.00 |
Slice Selection 3D/Bloch/CPU/2 thread(s) |
1583519021 ns |
1587890915.5 ns |
1.00 |
Slice Selection 3D/Bloch/CPU/4 thread(s) |
874887134 ns |
874868400 ns |
1.00 |
Slice Selection 3D/Bloch/CPU/8 thread(s) |
554811207 ns |
552711225 ns |
1.00 |
Slice Selection 3D/Bloch/CPU/1 thread(s) |
3014104944 ns |
3043255379.5 ns |
0.99 |
Slice Selection 3D/Bloch/GPU/CUDA |
31425726.5 ns |
30774441 ns |
1.02 |
Slice Selection 3D/Bloch/GPU/oneAPI |
117738422 ns |
117283108 ns |
1.00 |
Slice Selection 3D/Bloch/GPU/Metal |
112764792 ns |
108879500 ns |
1.04 |
Slice Selection 3D/Bloch/GPU/AMDGPU |
31907102 ns |
31712397 ns |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
| - `pulseq_version`: (`::VersionNumber`) Pulseq version | ||
|
|
||
| # Keywords | ||
| - `simplify_shapes`: (`::Bool`) whether to simplify the shapes of the gradients and RFs |
There was a problem hiding this comment.
Is this simplify_shapes argument also available on Pulseq? Is that the same as waveform compression? If so I would try to use the same name.
There was a problem hiding this comment.
It is not the same as waveform compression. This is Koma-specific and is intended to simplify Grad and RF events when possible. For instance, when an RF shape is defined in Pulseq as follows:
# Format of RF events:
# id ampl. mag_id phase_id time_shape_id center delay freqPPM phasePPM freq phase use
# .. Hz .. .. .. us us ppm rad/MHz Hz rad ..
# Field 'use' is the initial of:
# excitation refocusing inversion saturation preparation other undefined
[RF]
1 25 1 2 3 5000 0 0 0 0 0 e
# Sequence Shapes
[SHAPES]
shape_id 1
num_samples 2
1
1
shape_id 2
num_samples 2
0
0
shape_id 3
num_samples 2
0
10000
The read_seq function would create the RF instance like this:
rfAϕ = 25/γ .* [1, 1] .* exp.(1im*(2π*[0, 0]))
rf1 = RF(rfAϕ, [0, 10000] * rfRasterTime)that is, an RF with a two-point waveform.
However, for these cases where (a) the amplitude is constant along all time points and/or (b) all time points are equidistant, the RF instance can be simplified to:
rf2 = RF(25/γ, 10000 * rfRasterTime)The simplify_shapes keyword controls whether or not this simplification is applied when possible. By default, it is set to true, but it is disabled in "Read Comparison" tests so all tests pass (rf1 and rf2 are not formally the same).
The alternative to this is to always simplify (remove simplify_shapes kwarg), and define the ≈ method in a way that rf1 and rf2 are treated as equal.
Let me know what you think.
There was a problem hiding this comment.
Ok, simplifying makes sense (if this is explained somewhere). If its only simplifying this case, i would maybe call it differently.
| include("utils.jl") | ||
| pth = (@__DIR__) * "/test_files/pulseq/" | ||
| # EPI sequence | ||
| filename = pth * "round_trip_test.seq" |
There was a problem hiding this comment.
I would I add more tests than this, only comparing one sequence is probably not robust enough.
I am particularly interested in covering:
- RF with phase offset (RF spoiled)
- RF with frequency modulation (phase waveform, it could be an adiabtic pulse)
- RF with frequency modulation + phase offset (RF spoiled with slice offset)
- Gradient uniformly sampled waveforms
I briefly mentioned that when having a phase offset, the RF center is important. And that comparing complex waveforms to check uniqueness is not trivial. I would like to see some tests on this area.
| function round_trip_seq() | ||
| seq = Sequence() | ||
| seq += RF(1.0, 100e-6) | ||
| seq += Grad(1.0, 100e-6, 10e-6) | ||
| seq += ADC(100, 10e-6, 10e-6) | ||
| return seq | ||
| end |
|
Also something I completely forgot, but it would be great if scanner limits where checked before writing the file, to ensure that the sequence doesnt go beyond the hardware limits. |
2 participants
This PR addresses #152 and replaces #284 from @beorostica.
First of all, thanks to @beorostica for the original work and the time invested in that PR. Given the changes in master since then, I personally found it easier to reimplement the solution from scratch based on the current state of the master branch. The goal remains the same, with an updated implementation.
Round-trip tests (Koma
Sequence→write_seq→ .seq →read_seq→ KomaSequence) are still in progress. For now, a simple (RF+Grad+ADC) sequence is used, but it needs to be improved (more blocks, events, time-shaped waveforms, extensions, …).Some additional functions and keyword arguments have been added compared to #614. We could take a deeper look at them in a meeting.