-
Notifications
You must be signed in to change notification settings - Fork 31
Read Pulseq 1.5.1 #614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Read Pulseq 1.5.1 #614
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use three correct logic so pulseq 1.4 still works. No need to pass the version into the get_X functions, you can just check the input length of the library.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
==========================================
- Coverage 91.22% 90.89% -0.33%
==========================================
Files 57 61 +4
Lines 3248 3351 +103
==========================================
+ Hits 2963 3046 +83
- Misses 285 305 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
(1) Update get_events to add (2) Test with reader with (take inspiration from https://juliahealth.org/KomaMRI.jl/dev/explanation/5-seq-events/):
@Stockless can you give use the MATLAB files you were using for the tests? (3) Modify to consider new RF uses, to calculate k-space correctly (not guess RF use by > 90.0):
If use is Undefined(), keep using the current method; otherwise, use the provided value. If center is not defined (i.e., == 0.0), keep the current method; otherwise, use the provided value. |
|
@pvillacorta , @cncastillo mentioned you are interested in working this PR out. I want to get it done too. Let's meet to sort out the efforts? |
Yes! Let's join forces :) |
|
@pvillacorta @gsahonero do you guys want to plan a meeting to push this? |
|
@gsahonero and I are meeting this thursday at 4pm (spanish time). I hope we can plan how to finish this PR and the one for writing Pulseq. |
- Improve Grad and RF constructors for improved readability and error handling. - Fix documentation typo about `A` and `T` vectors - Add SHA and MD5 packages for hash generation/verification - Improve signature generation/verification
- Add missing signature-related functions - Simplify `read_RF` and `read_ADC` in the same way as `read_Grad`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KomaMRI Benchmarks
Details
| Benchmark suite | Current: 2efa648 | Previous: 11d3180 | Ratio |
|---|---|---|---|
MRI Lab/Bloch/CPU/2 thread(s) |
334561715.5 ns |
337633469 ns |
0.99 |
MRI Lab/Bloch/CPU/4 thread(s) |
278611014 ns |
276282606 ns |
1.01 |
MRI Lab/Bloch/CPU/8 thread(s) |
279866990.5 ns |
209852165 ns |
1.33 |
MRI Lab/Bloch/CPU/1 thread(s) |
559995522 ns |
555065232 ns |
1.01 |
MRI Lab/Bloch/GPU/CUDA |
20997855 ns |
21231134 ns |
0.99 |
MRI Lab/Bloch/GPU/oneAPI |
73292888 ns |
77053690 ns |
0.95 |
MRI Lab/Bloch/GPU/Metal |
92637208 ns |
95540333 ns |
0.97 |
MRI Lab/Bloch/GPU/AMDGPU |
23963376.5 ns |
24763685 ns |
0.97 |
Slice Selection 3D/Bloch/CPU/2 thread(s) |
1571567038 ns |
1592087059 ns |
0.99 |
Slice Selection 3D/Bloch/CPU/4 thread(s) |
888550775 ns |
889539516 ns |
1.00 |
Slice Selection 3D/Bloch/CPU/8 thread(s) |
567568116 ns |
565401528.5 ns |
1.00 |
Slice Selection 3D/Bloch/CPU/1 thread(s) |
3038525168.5 ns |
3029269071 ns |
1.00 |
Slice Selection 3D/Bloch/GPU/CUDA |
31434342 ns |
32639703 ns |
0.96 |
Slice Selection 3D/Bloch/GPU/oneAPI |
118202439 ns |
121489006.5 ns |
0.97 |
Slice Selection 3D/Bloch/GPU/Metal |
108188250 ns |
111152750 ns |
0.97 |
Slice Selection 3D/Bloch/GPU/AMDGPU |
31967231 ns |
32636098 ns |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
- Fix contructors, auxiliary functions and tests - Restore previous `read_Grads`, `read_RF`, and `read_ADC` implementations - Improve signature handling
|
Pulseq files from |
|
I have modified and created new MATLAB scripts into a local version of this folder in order to make similar (v1.5.1) sequences to those existing in v1.4. |
- Moved `AdcLabels`, `LabelInc`, `LabelSet`, and `Trigger` into separate files for better organization. - Updated `EXT.jl` to include new extension files and added functions to retrieve extension types from symbols. - New `get_scanf_format` function to generate format strings based on struct field types. - Modified `read_events` and `read_extension` functions to accommodate the new structure and improve extensibility. - Updated `read_seq` to handle new extension libraries and ensure compatibility with existing sequence files.
|
The commit above tries to address #575. The main change is that it is no longer necessary to create a library for each extension type inside the reader.
Maybe leave them for another PR? mutable struct SoftDelay <: Extension
num::Int
offset::Int
factor::Int
hint::String
endLet me know what you think, I hope we can merge this soon in order to also address #152 during the following days. |
cncastillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I jut wanted to say, that the introduction of trapLibrary and other dictionaries seem to deviate too much from the MALTAB's Pulseq reader. I would like to follow the same logic, even if inefficient so it is easier to update when new versions are released.
Also, the whole handling of fmt strings for scanf (like the function get_scanf_format) seem overly complicated. The comment I left gives a solution, but there might be an easier one like:
struct MyStructure
x::Float64
y::Int
end
t = MyStructure(parse.(fieldtypes(MyStructure), split("1.0 1"))...)The extensive use of Dict's to map inputs to types seems weird to me, and all the generated arrays of type Any are not very performant.
Finally, I think there is too many functions like f(x::Type{T}) instead of f(x::T) which seems too complicated.
I haven't reviewed the whole PR, but I don't think in the current state in can be merged quickly, there are too many "unnecessary" changes. For example, there are some breaking changes on how to read Pulseq <1.4.
My main point: Let's try to stick to whatever Pulseq is doing, let's only deviate for performance or extensibility.
- Remove `get_scanf_format` and `get_scanf_args` - Reduce the number of type Any arrays - Put `get_EXT_type_from_symbol` implementations inside their corresponding file
… into pulseq1.5.0
I agree that this deviates from the exact structure of the MATLAB reader. The main motivation for introducing
By separating trapezoidal gradients from arbitrary gradients into a dedicated library, the dispatch logic in At this point, I see this as a pragmatic compromise: it deviates slightly from MATLAB’s implementation, but significantly reduces code complexity and improves maintainability. My intention was not to redesign the Pulseq logic, but to make the Julia reader easier to reason about and extend while keeping the mapping to Pulseq concepts clear.
You’re absolutely right here, and I’ve already addressed this in the latest commit. I removed the generic auxiliary functions (
This is a fair concern, especially regarding performance. Here I intentionally tried to stay close to what was already present in the reader before this PR. Dictionaries were already used extensively, and keeping them allowed me to:
I fully agree that performance could be improved by using more concrete types (or even arrays directly, as it was stated in #224). However, given time constraints and the goal of not overengineering this PR, I preferred to keep consistency with the existing codebase rather than introduce a larger refactor.
The only places where I dispatch on Instantiating a dummy object before parsing its arguments felt less practical (and potentially more expensive) than dispatching on the type directly. While I agree this is not ideal in general, in this context it seemed like a reasonable trade-off to keep the overall logic simpler and more uniform.
I understand this concern, but I’m not entirely sure which specific breaking changes for Pulseq < 1.4 you’re referring to. If you can point out a concrete example or file, I would try to adjust or revert that part.
I fully agree with your main point. we should stick to Pulseq's logic and only deviate when there is a clear benefit in terms of performance or extensibility. My intention was exactly that, not to redesign the reader functions, but to make the Julia reader usable, maintainable and more extensible. Given the time constraints, I think this is a reasonable compromise, and the upcoming Pulseq writer PR will follow the same direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I fully reviewed the PR, I think it is pretty good. Most of my comments are just to verify that we match what Pulseq does.
KomaMRIFiles/test/runtests.jl
Outdated
| @test first(ev_koma.A) ≈ first(ev_pulseq.A) || ev_koma.t[2] ≈ ev_koma.t[1] | ||
| @test last(ev_koma.A) ≈ last(ev_pulseq.A) | ||
| pth = joinpath(@__DIR__, "test_files/pulseq/pulseq_read_comparison/") | ||
| versions = ["v1.4", "v1.5"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No tests for versions lower than 1.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I currently don’t have an easy way to generate test data for Pulseq < 1.4. Supporting it would require setting up older MATLAB versions and regenerating .seq and .mat files. This is doable, but I wasn’t sure whether <1.4 is still actively used (e.g., by JEMRIS). Happy to add this if it’s considered important.
- Remove `skip_rf` - Modify `RF` constructor: `_RF_with_center` -> `_RF_with_center_and_use` - Adapt `get_RF_center` to what Pulseq does - Change `rf-pulse.seq` (and .mat) to consider a non-zero delay - "RFs" labels -> "RF_center" - Rename all dictionary-reading functions (`read_*`->`get_*`) - Simplify version handling in `get_Grad`, `get_RF` and `get_ADC` - Dispatch for unknown extensions - Display RF center in `plot_seq` (new `cents` aux function) - New `has_hash` variable - Dispatch depending on signature existance - Remove `blockDurations` and `delayInd_tmp` dictionaries - Recover 't' and 'g' gradient types, delete `trapLibrary`
|
@cncastillo I have resolved all the comments that I could address by myself. |
- Fix `fix_first_last_grads!` to mimic Pulseq's MATLAB implementation - New `simplify_waveforms` function - Fix RF center representation in `plot_seq`: - Remove useless `cents` function - Introduce `cents` functionality into `times` function - Add previous blocks timing information - Better distribution of KomaFiles test files
|
I made the last commit in order to (we can talk about it more in detail in a meeting if you consider):
|
|
I think we might need to organize a meeting there's too many changes. I don't understand why the Also I noticed that some of the tests use The functions Can you explain the bug you encountered with |
- finish `fix_first_last_grads!` - Remove debugging comments - Recover previous `get_Grad` function - Simplify tests (not use `inside` function)
- Simplify plot centers in `plot_seq` - Add warning when a gradient time shape starts at a non-zero value. - Update info message regarding v1.5 support
|
This comment summarizes the points discussed in yesterday’s meeting with @cncastillo and their current status after the latest commit (9c84075). Some items required code changes (included in the commit), while others were already resolved and have now been verified:
Additionally, I have changed the information message that appears when loading a v1.5 sequence: ┌ Info: Pulseq 1.5.1 is supported, but Soft Delay, Rotation, and RF Shimming extensions are not yet included
└ (see https://github.com/JuliaHealth/KomaMRI.jl/issues/714)The new issue #714 accounts for Pulseq v1.5 extension support. |
cncastillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any test failing!
I think that this is not intuitive, and introducing pulseq version as an get_block input argument is totally worth it.
Ok!
Additionally, I have changed the information message that appears when loading a v1.5 sequence:
I would only give a warning if the user uses any of the non-implemented extensions. As it will become annoying for people to now use them. Or at least, maxlog=1.
| A=reduce(vcat, [usrf(block.rf.A); Inf] for block in seq_samples), | ||
| t=reduce(vcat, [usrf(block.rf.t); Inf] for block in seq_samples), | ||
| ct=center_times, | ||
| cA=abs.(KomaMRIBase.get_rfs(seq, center_times)[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another important parameter to show at the RF center is the phase.
| p[2j + 5] = scatter_fun(; | ||
| x=rf.ct * 1e3, | ||
| y=rf.cA * 1e6 * frf, | ||
| name="RF_center", | ||
| hovertemplate="RF center: %{x:.4f} ms<br>Amplitude: %{y:.2f} μT<extra></extra>", | ||
| xaxis=xaxis, | ||
| yaxis=yaxis, | ||
| legendgroup="RF_center", | ||
| showlegend=showlegend, | ||
| mode="markers", | ||
| marker=attr(; color="#FF0000", symbol="x"), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For very long sequences (> 1000 RFs) can you check if this is too slow?, maybe the center display can be turned off by default.

This PR addresses issue #555 by adding support for reading Pulseq files in version 1.5.0.