Add support for speed up in UTE trajectories #277
Conversation
paquiteau
left a comment
paquiteau
left a comment
There was a problem hiding this comment.
Nice trick with the decorator, but it brings its own challenges, see my comments about docstrings notably.
A nice example comparing the different cases will greatly help to understand why and when enabling this feature.
| import inspect | ||
| from functools import wraps |
| slew_ramp_disable : bool, optional | ||
| If True, disables the slew ramp and returns the trajectory as is, | ||
| by default False. This is useful for in-out trajectories where | ||
| the slew ramp is not needed. |
There was a problem hiding this comment.
Well I would make it default to None, and if is None, use the value of in-out then ?
There was a problem hiding this comment.
Hmm, but this is needed when we have some center-out trajectories which calls internally for another trajectory (with center-out), and in that case we dont want UTE codes. In particular, i remember some radial trajectories use spiral end points...
src/mrinufft/trajectories/tools.py
Outdated
| unnormalized_traj = unnormalize_trajectory(traj, resolution=_resolution) | ||
| gradients, initial_positions = convert_trajectory_to_gradients( | ||
| traj, resolution=_resolution, raster_time=_raster_time, gamma=_gamma | ||
| ) | ||
| gradients_to_reach = gradients[:, _ramp_to_index] | ||
| # Calculate the number of time steps for ramps | ||
| n_ramp_down, n_ramp_up, n_plateau, gi = get_gradient_times_to_travel( | ||
| kspace_end_loc=unnormalized_traj[:, _ramp_to_index], | ||
| end_gradients=gradients_to_reach, | ||
| gamma=_gamma, | ||
| raster_time=_raster_time, | ||
| smax=_smax, | ||
| n_jobs=-1, # Use all available cores | ||
| ) | ||
| # Update the Ns of the trajectory to ensure we still give | ||
| # same Ns as users expect. We use extra 2 points as buffer. | ||
| n_slew_ramp = np.max(n_ramp_down + n_ramp_up + n_plateau) | ||
| bound.arguments["Ns"] -= n_slew_ramp - _ramp_to_index | ||
| new_traj = trajectory_func(**bound.arguments) | ||
|
|
||
| # Re-calculate the gradients | ||
| unnormalized_traj = unnormalize_trajectory(new_traj, resolution=_resolution) | ||
| gradients, initial_positions = convert_trajectory_to_gradients( | ||
| new_traj, resolution=_resolution, raster_time=_raster_time, gamma=_gamma | ||
| ) | ||
| gradients_to_reach = gradients[:, _ramp_to_index] | ||
| ramp_up_gradients = get_gradient_amplitudes_to_travel_for_set_time( | ||
| kspace_end_loc=unnormalized_traj[:, _ramp_to_index], | ||
| end_gradients=gradients_to_reach, | ||
| nb_raster_points=n_slew_ramp, | ||
| gamma=_gamma, | ||
| raster_time=_raster_time, | ||
| smax=_smax, | ||
| n_jobs=-1, # Use all available core | ||
| )[:, :-1] | ||
| ramp_up_traj = convert_gradients_to_trajectory( | ||
| gradients=ramp_up_gradients, | ||
| initial_positions=initial_positions, | ||
| resolution=_resolution, | ||
| raster_time=_raster_time, | ||
| gamma=_gamma, | ||
| ) | ||
| return np.hstack([ramp_up_traj, new_traj[:, _ramp_to_index:]]) |
There was a problem hiding this comment.
I would extract this in a dedicated function, so that it can be applied separately if needed.
There was a problem hiding this comment.
Thats the idea, you can call add_slew_ramp also if you want to directly...
There was a problem hiding this comment.
I get that, its just for the clarity of the code, clearly separating the compute part from the decorator stuff. Typically put all the compute stuff in a _add_slew_ramp_to_traj private-ish function.
src/mrinufft/trajectories/tools.py
Outdated
| """Add slew-compatible ramps to a trajectory function. | ||
| This decorator modifies a trajectory function to include | ||
| slew rate ramps, ensuring that the trajectory adheres to | ||
| the maximum slew rate and gradient amplitude constraints. | ||
| The ramps are applied to the gradients of the trajectory | ||
| at the specified `ramp_to_index`, which is typically the | ||
| index of the first readout sample. |
There was a problem hiding this comment.
Its not clear here if the length of the trajectory is modified. From the code below we see how, but this should be made explicit, and ideally refer to an example comparing the with/without slew ramps cases.
| sig = inspect.signature(trajectory_func) | ||
|
|
||
| @wraps(trajectory_func) | ||
| def wrapped(*args, **kwargs) -> NDArray: |
There was a problem hiding this comment.
Because this decorators adds arguments to existing functions, they need to be documented as well (and appear in the online documentation). I think you can use the inspect module and/or the __doc__ attribute to modify the docstring on the fly, typically to replace a %hardware_args placeholder in the original docstring
There was a problem hiding this comment.
Ahhh, do we want to do that? I guess yes.. But I need to readup about it.
There was a problem hiding this comment.
See for instance: https://github.com/paquiteau/patch-denoising/blob/master/src/patch_denoise/_docs.py (I remember also seeing this in nilearn)
Co-authored-by: Pierre-Antoine Comby <[email protected]>
|
@paquiteau how much of this shall change with your new PRs? I might just convert this to draft and re-do it over your changes if so. |
paquiteau
force-pushed
the
master
branch
3 times, most recently
from
0ea2bc4 to
d338014
Compare
2 participants
This resolves #273 .
I wrote a custom wrapper to for UTE.