Display 3D trajectories, with a grid interface #193
Conversation
This add "get_interpolators_from_fieldmap", supporting both real (B0 map only) and complex (Zmap = R2* + 1j * B0map) fields in arbitrary dimension (2D and 3D). The routine supports both numpy / cupy arrays and torch tensors on CPU and GPU, the latter requiring cupy due to limitations in torch.histogram / torch.histogramdd (pytorch/pytorch#69519). Calculation uses time segmentation with uniform time samples and LS coefficients (using histogram). Based on MIRT (https://github.com/JeffFessler/mirt/blob/main/mri/mri_exp_approx.m) and its Python porting from MIRTORCH (https://github.com/guanhuaw/MIRTorch/blob/master/mirtorch/linear/mri.py) and SigPy (https://github.com/mikgroup/sigpy/blob/main/sigpy/mri/util.py).
In addition, add off-resonance example.
Avoid torch cuda test case if cupy is not available.
remove deprecated get_grad Co-authored-by: Chaithya G R <[email protected]>
|
I will get a CLI also :P |
|
I seemed to have forgotten to add the example. Added it now. @Daval-G you are good to go for review whenever you get time. |
|
@chaithyagr I will have a look over the week-end |
| def create_grid(grid_type, title="", **kwargs): | ||
| fig, axs = plt.subplots(3, 3, figsize=(10, 10)) | ||
| for i, (name, traj) in enumerate(trajectories.items()): | ||
| grid = get_gridded_trajectory( | ||
| traj, (64, 64, 64), grid_type=grid_type, traj_params=traj_params, **kwargs | ||
| ) | ||
| plot_slices(axs[:, i], grid, title=name) | ||
| plt.tight_layout() | ||
| plt.suptitle(title) | ||
| plt.show() |
There was a problem hiding this comment.
Why not provide trajectories and traj_params as arguments but keep grid_type and title ? It makes it a bit of a pain to read here and below
There was a problem hiding this comment.
I moved title below. But I dont know if I understand you?
There was a problem hiding this comment.
I meant that some of the variable used in the create_grid function are taken as argument and some others are just assumed to be there in the global context of execution, and I don't get the logic. Why not just put everything as an argument like a usual function ?
| data = grid_op.raw_op.adj_op( | ||
| np.tile(np.linspace(1, 10, shots.shape[1]), (shots.shape[0],)), | ||
| None, | ||
| True, | ||
| ) |
There was a problem hiding this comment.
Don't we want to provide actual time values ? It should be more explicit in the documentation
There was a problem hiding this comment.
Also the output shape doesn't correspond to shape as expected when reading the documentation. I don't understand why this is part of that function since it doesn't relate to a grid
There was a problem hiding this comment.
It is just relative to get an understanding. Given that adjoint is not inverse, even if we give exact values, we cant expect it to be reflected the same way on image domain.
The shape would still be same as expected as we use grid_op which is set based on shape, right? Am I missing something?
There was a problem hiding this comment.
Good point, thx !
Still curious, why 1 to 10 ? Why not just 0 to 1 ? At this point it doesn't matter much but I wonder
|
|
||
| Parameters | ||
| ---------- | ||
| shots : ndarray |
There was a problem hiding this comment.
The shots name isn't conventional in that part of the package
There was a problem hiding this comment.
What would you recommend? traj ?
There was a problem hiding this comment.
trajectory as in all other files
| def get_gridded_trajectory( | ||
| shots: np.ndarray, | ||
| shape: Tuple, | ||
| grid_type: str = "density", | ||
| osf: int = 1, | ||
| backend: str = "gpunufft", | ||
| traj_params: dict = None, | ||
| turbo_factor: int = 176, | ||
| elliptical_samp: bool = True, | ||
| threshold: float = 1e-3, | ||
| ): |
There was a problem hiding this comment.
Overall, why have all of those features gathered into only one function ? Some of them are not related to grids.
There was a problem hiding this comment.
Also please add more comments to your code
There was a problem hiding this comment.
I think, all the features are related to gridding and how you want to "color" the trajectories on the grid.
Co-authored-by: Guillaume Daval-Frérot <[email protected]>
|
@Daval-G is there something else remaining or am I missing something here? |
| def create_grid(grid_type, title="", **kwargs): | ||
| fig, axs = plt.subplots(3, 3, figsize=(10, 10)) | ||
| for i, (name, traj) in enumerate(trajectories.items()): | ||
| grid = get_gridded_trajectory( | ||
| traj, (64, 64, 64), grid_type=grid_type, traj_params=traj_params, **kwargs | ||
| ) | ||
| plot_slices(axs[:, i], grid, title=name) | ||
| plt.tight_layout() | ||
| plt.suptitle(title) | ||
| plt.show() |
There was a problem hiding this comment.
I meant that some of the variable used in the create_grid function are taken as argument and some others are just assumed to be there in the global context of execution, and I don't get the logic. Why not just put everything as an argument like a usual function ?
| # Display the density of the trajectories, along the 3 mid-planes. For this, make `grid_type="density"`. | ||
| create_grid("density", "Sampling Density") |
There was a problem hiding this comment.
@chaithyagr Thanks for addressing the second comment. The first one is still up to date, let me know your opinion
|
|
||
| Parameters | ||
| ---------- | ||
| shots : ndarray |
There was a problem hiding this comment.
trajectory as in all other files
| elliptical_samp : bool, optional | ||
| Whether to use elliptical sampling. Default is True. | ||
| This is useful while analyzing the k-space holes. |
There was a problem hiding this comment.
I will rephrase: "Whether to use elliptical sampling" sounds odd, we already provide the trajectory so it's not like we will decide to use elliptical sampling. Is it that we tell if we sampled the k-space corners ? Then please explicit it, and tell why we need to provide it, what is it going to change ?
Please rephrase "This is useful while analyzing the k-space holes, especially if the k-space trajectory is expected to be elliptical sampling of k-space", we are missing words and the formulation could be simpler
| def get_gridded_trajectory( | ||
| shots: np.ndarray, | ||
| shape: Tuple, | ||
| grid_type: str = "density", | ||
| osf: int = 1, | ||
| backend: str = "gpunufft", | ||
| traj_params: dict = None, | ||
| turbo_factor: int = 176, | ||
| elliptical_samp: bool = True, | ||
| threshold: float = 1e-3, | ||
| ): |
|
Hopefully have tackled all the queries. Its getting meesy to keep track of, @Daval-G please have a look, I did not resolve any comments.. and ofcourse, thank you for the review. |
No description provided.