Open
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 Additional details and impacted files@@ Coverage Diff @@
## master #718 +/- ##
==========================================
- Coverage 91.11% 90.61% -0.51%
==========================================
Files 62 64 +2
Lines 3389 3986 +597
==========================================
+ Hits 3088 3612 +524
- Misses 301 374 +73
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}
Contributor
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.
cncastillo
requested changes
Jan 28, 2026
Member
|
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. |
- Enhance the `≈` operator for `Sequence` to improve event comparison, including handling of empty events and waveform interpolation. - Update `write_seq` to return the correct ADC delay and add detailed comments for clarity. - Improve the `register_grad!` function to handle various waveform types. - Improve a bit (WIP) round trip test. - Rename test directories.
- Improve phase offset calculations and ensure correct scaling of values - Bump to 0.9.10 - Add `get_symbol_from_EXT_type` function
- `phase_equality` -> `phase_shape` - Remove unnecessary brackets
Closed
- New function `check_scanner_contraints`.
Export from KomaBase
- Modify `quantize_to_pulseq` -> `check_raster`;
it now adapts the sequence to the Pulseq raster correctly.
Export from KomaFiles
- Get `DEFAULT_RASTER` from default`Scanner` values
- Changes in Pulseq.jl:
- Remove `seq` field from `PulseqExportContext` struct.
- Add `definitions` field to `PulseqExportContext` struct.
- New constructor for `PulseqExportContext`
- New function `get_RasterTime`
- Remove `trapezoids` from `PulseqExportAssets` struct
- Const types `ArbitraryGradient` and `TrapezoidGradient`
- New function `check_raster`
- Add information messages and doble cheking in `write_seq`
- Unexport `check_raster` - Add `check_scanner_constraints` to docs - Fix Printf version dependency problem - Change tolerance for sequence comparison - More appropriate block duration rounding - Add new sequences to tests: - Uniformly-shaped RF - Time-shaped RF - RF with phase offset - Adiabatic RF
Collaborator
Author
|
I have tried to cover most of the requested details. I think the only pending changes are:
|
Member
|
Thanks! I tested it on this sequence: Read takes 6ms, write takes 70ms (~10x). I also did a "roundtrip" test: but the reader expect so there is an error in the type and unit conversions as well. Also, the saved DEFINITIONS are weird (we may want to ignore some |
Member
|
Also, right now there are some pretty big differences on the output format. Like hard coding two spaces I would also suggest to split Pulseq.jl to PulseqRead.jl and PulseqWrite.jl. |
…nd update `get_slew_rate` and `get_eddy_currents` to use it. Enhance error messages in `check_scanner_constraints` for better clarity.
- Fix Trigger output format (Int) - (Cursor) Output format improvements
Collaborator
Author
|
I made a set of improvements related to your comments:
|
- Remove unsed `store_shape` and `store_event` methods - Rename keyword to adapt to format - Test coverage for all types of signature algorithms
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #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.