-
Notifications
You must be signed in to change notification settings - Fork 19
Pulseq #283
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?
Conversation
src/mrinufft/io/pulseq.py
Outdated
| # TODO ensure amplitudes are correct | ||
|
|
||
| # TODO deduplicate gradients (reduces the size of the file, speed up the sequence loading). | ||
| # TODO Are rotation of trajectories supported in pulseq? |
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.
yes :) https://github.com/imr-framework/pypulseq/blob/master/src/pypulseq/rotate.py
But i wonder why you need it. The rotation of trajectory to match the GOV rotation is done by the sequence at scanner.
src/mrinufft/io/pulseq.py
Outdated
| # TODO add prewind/postwind stuff from #276 | ||
| # TODO ensure amplitudes are correct | ||
|
|
||
| # TODO deduplicate gradients (reduces the size of the file, speed up the sequence loading). |
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 dont know what does this mean? No matter what, i think you need to give all the gradients anyway. thats the downside of pulseq really.. u cant exactly tell run this trajectory and rotate it and run again etc.
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.
yes that was the idea behind this see for more detail: imr-framework/pypulseq#184
src/mrinufft/io/pulseq.py
Outdated
| for grad_shot_id in grad_shot_ids: | ||
| pp.add_block() # RF pulse | ||
| pp.add_block() # pause for tuning echo time | ||
| pp.add_block() # Gradient block with ADC |
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.
| pp.add_block() # Gradient block with ADC | |
| pp.add_block() # Gradient block with ADC | |
| pp.add_block() # Gradient spoiler block | |
| pp.add_block() # pause for tuning for TR |
|
So the example is running well for the 3D GRE case, I think we could cover more cases (A 2D slice selective GRE first). working on top of #276 was the right choice to get very nice prewind/prephasor and rewind gradients. Currently the sequence is generated for a given FOV and resolution, I don't know if Pulseq can provide some flexibility on this or not (e.g can you parametrized it at the console for instance, to change TE/TR/FA, or the resolution/fov) @Daval-G do you have any comments ? |
chaithyagr
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.
Great work. Some major questions and minor reviews. I will need another round of review.
| seq = pulseq_gre_3D( | ||
| trajectory=kspace_loc, fov=FOV, img_size=img_size, TR=1000, TE=100, FA=90 | ||
| ) |
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.
Maybe you can also add checks to see if TE, TR and FA are satisfied?
| def _check_timings(seq): | ||
| # Check whether the timing of the sequence is correct | ||
| ok, error_report = seq.check_timing() | ||
| if ok: | ||
| return None | ||
| else: | ||
| warnings.warn("Timing check failed. Error listing follows:" + str(error_report)) |
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.
Why a separate function really?
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.
Cause I am planning to add other sequences (like a 2D GRE for instance) this would be common to all of them.
src/mrinufft/io/pulseq.py
Outdated
| def pulseq_gre_3D( | ||
| trajectory: NDArray, |
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.
Why not have a generic one for both 2D and 3D?
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.
2D GRE have some specificities (specific pulse and slice selection that changes over time), it makes more sense to me to have it separately (some core will be refactor if needed)
| delay_before_grad = pp.make_delay( | ||
| TE | ||
| - pp.calc_rf_center(rf_pulse)[0] | ||
| - rf_pulse.delay | ||
| - TE_pos * shot_duration | ||
| - skip_start * system.grad_raster_time | ||
| ) | ||
| delay_end_TR = pp.make_delay( | ||
| TR | ||
| - pp.calc_duration(rf_pulse) | ||
| - delay_before_grad.delay | ||
| - full_grads.shape[1] * system.grad_raster_time | ||
| ) |
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.
You might need to check for min_TE and min_TR. Can you add a util function to get it?
Basically for a fixed trajectory and spoil levels, give the minimum TE and TR possible.
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.
this is handled by the check_timing function of pulseq (there is check for delay that can't be negative for instance)
|
This will ideally depend on #285 eventually? |
Co-authored-by: alexis <[email protected]> Update examples/example_pulseq.py Co-authored-by: Chaithya G R <[email protected]>
Co-authored-by: Pierre-Antoine Comby <[email protected]>
fix: remove type annonation for optional deps. Otherwise, its a mess. Acquisition Configuration (#285) * feat!: acquisition Config. This is a breaking change! * fmt: black * feat(test): use acquisition fixture for tests. * feat: docstrings * refactor!: use acq in display functions as well * fmt:black * fix: correct field name. * feat: move adc setup to acquisition level. * fix: example use the acquisition object * feat: example use acquisition. * fix: style * use correct units * fix: enum meta has evolved in python 3.12 * feat: use better default for grad raster time. * feat: more SI units and acq use * fix: don't modify API for write/read_trajectory * fmt * fix examples * update * fix: acq and display * feat: add context manager for the acquisition * update context manager * fmt: PEP604 type union fixes UP045 * [docs] update docstring for acquisition config.
0ea2bc4 to
d338014
Compare

Changes proposed in this pull request:
Add a reader for pulseq sequences to extract kspace trajectories (formatted to MRI-nufft conventions) from pulseq files
traj -> write_seq -> read_seq -> compare.( Add a helper function to create gradients block for pypulseq)prepare_trajectory_for_seqthat creates gradients waveforms with pre/re-winders. Generating the gradients event directly does not make much sense I feel.Add a simple generation of a GRE sequence with pypulseq.
More sequence type support ?