-
Notifications
You must be signed in to change notification settings - Fork 12
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
Display 3D trajectories, with a grid interface #193
base: master
Are you sure you want to change the base?
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]>
Co-authored-by: Guillaume Daval-Frérot <[email protected]>
Please base yourself from master :) |
Naa, ill wait for #180 to go in :P |
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 |
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.
It's a bunch of useful features that will make a great addition, but please try to make their use cases clearer to the readers.
Also remember to update the __init__.py
files
@@ -0,0 +1,99 @@ | |||
""" | |||
====================== | |||
3D Trajectory Display |
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.
3D Trajectory Display | |
3D Trajectory display |
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.
Also why is it a GPU example ?
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.
@chaithyagr Sorry I misplaced this comment, but could you answer it ?
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 think the reason why it's a GPU example is cause we use spread and interpolate kernels which currently is only supported on GPU backends. I'll add support for it in finufft soon
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved title below. But I dont know if I understand you?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to provide actual time values ? It should be more explicit in the documentation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 1 to 10 ?
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.
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?
|
||
Parameters | ||
---------- | ||
shots : 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.
The shots
name isn't conventional in that part of the package
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.
What would you recommend? traj
?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please add more comments to your code
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 think, all the features are related to gridding and how you want to "color" the trajectories on the grid.
data = grid_op.raw_op.adj_op( | ||
np.repeat( | ||
np.linspace(1, 10, turbo_factor), samples.shape[0] // turbo_factor + 1 | ||
)[: samples.shape[0]], | ||
None, | ||
True, | ||
) |
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.
Same problem here, the output shape doesn't seem to match the documented output
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.
Same as above, isnt data.shape same as shape we give as input?
if elliptical_samp: | ||
data[ | ||
np.linalg.norm( | ||
np.meshgrid( | ||
*[np.linspace(-1, 1, sh) for sh in shape], indexing="ij" | ||
), | ||
axis=0, | ||
) | ||
> 1 | ||
] = 0 |
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.
Please add comments
@@ -0,0 +1,99 @@ | |||
""" | |||
====================== | |||
3D Trajectory Display |
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.
Also why is it a GPU example ?
Co-authored-by: Guillaume Daval-Frérot <[email protected]>
No description provided.