-
Notifications
You must be signed in to change notification settings - Fork 75
[towards v1.5.x] Bump RF storage to v1.5.x #291
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
Conversation
|
Yes, this is great, thank you! I will have a look at this in detail, soon. Meanwhile a thought: should we have tests that also checks the sequences written in |
|
Good idea! I guess we'll need multiple expected output files for the different storage formats |
|
On a second thought, do we really want to support writing to older formats? I know MATLAB toolbox does, but doesn't that introduce a lot of maintainance overhead? After all, we do not have, let's say, write_131(). Just thinking out loud here, if we decide it is worth to support older revisions I am happy to include the corresponding tests. |
|
I see what you mean. People who did not migrate their interpreters to 1.5 can stick with an older version of PyPulseq. I guess it boils down to the question, if we want people to use a newer version PyPulseq regardless, for them to have access to bug fixes and some of the new features. We can also decide this down the line, probably. |
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.
Pull Request Overview
This PR bumps the RF storage format to v1.5.0 and updates the internal representation of RF pulses by adding new fields (center, freq_ppm, phase_ppm) in key modules such as sequence.py, block.py, and read_seq.py. Additional changes include updates to expected output files and modifications to various pulse‐creation routines and helper functions to accommodate the new RF event tuple format.
- Updated RF library tuple structure and its handling in sequence, block, and read_seq modules.
- Added new parameters (freq_ppm, phase_ppm) and default values (e.g. use='undefined') to pulse‐creation routines.
- Minor updates to test outputs and examples to include the new attributes.
Reviewed Changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/* | Expected output files updated to reflect new RF event format changes |
| src/pypulseq/supported_labels_rf_use.py | Updated supported RF use labels by including 'other' and 'undefined' |
| src/pypulseq/make_*_pulse.py | Added new parameters (freq_ppm, phase_ppm) and updated docstrings accordingly |
| src/pypulseq/Sequence/(sequence.py, block.py, read_seq.py) | Modified RF storage handling and tuple mappings to support v1.5.0 format |
| pyproject.toml | Bumped project version from 1.4.2.post1 to 1.5.0 |
| examples/scripts/* | Updated examples to pass explicit use parameters |
Comments suppressed due to low confidence (3)
src/pypulseq/make_block_pulse.py:20
- The RF use validation logic is repeated across several pulse‐creation functions. Consider factoring this out into a shared helper function to ensure consistency and simplify future updates.
system: Union[Opts, None] = None,
src/pypulseq/Sequence/sequence.py:1286
- The TODO comment suggests that the RF use mapping should be derived from get_supported_rf_uses() instead of being hardcoded. Please update or remove the TODO once a dedicated mapping mechanism is implemented.
# TODO: fixme : use map built from pp.get_supported_rf_uses()
src/pypulseq/make_gauss_pulse.py:70
- The function's docstring should be updated to include and explain the new parameters (freq_ppm and phase_ppm) added to the RF event structure.
slice select event.
btasdelen
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.
Just added two minor comments to be addressed, other than that looks good. Can be merged afterward.
This PR partially addresses #258.
This only handles changes to RF storage format. The most substantial updates concerns
sequence.py,block.pyandread_seq.py/write_seq.pymodules. Other changes are either very minor updates to include the new attributes in the pulse design routines (make_*.py) or are re-generation of the expected output for the test cases.For the moment, I excluded a few tests failing due to some inconsistencies in gradient storage format when writing and reading again. I am planning to re-enabling them when bumping gradient storage format.
@btasdelen do you think this is small enough or should I try to further divide this? Thanks!