-
Notifications
You must be signed in to change notification settings - Fork 9
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
Test multi-coils in test_autodiff #139
Conversation
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.
Thank you for this great work! Minor changes
src/mrinufft/operators/autodiff.py
Outdated
grid_x = x * grid_r # Element-wise multiplication: x * r | ||
|
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.
Lets not add unwanted space.
src/mrinufft/operators/autodiff.py
Outdated
grad_traj = torch.cat( | ||
[torch.transpose((-1j * torch.conj(dy[:, i, :]) * nufft_dx_dom[:, i, :]), 0, 1)[None, ...] for i in range(dy.shape[1])], | ||
dim=0, | ||
) | ||
|
||
grad_traj = torch.mean(grad_traj, dim=0).type_as(traj) | ||
|
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.
grad_traj = torch.cat( | |
[torch.transpose((-1j * torch.conj(dy[:, i, :]) * nufft_dx_dom[:, i, :]), 0, 1)[None, ...] for i in range(dy.shape[1])], | |
dim=0, | |
) | |
grad_traj = torch.mean(grad_traj, dim=0).type_as(traj) | |
grad_traj = torch.mean( | |
[torch.transpose((-1j * torch.conj(dy[:, i, :]) * nufft_dx_dom[:, i, :]), 0, 1)[None, ...] for i in range(dy.shape[1])], | |
dim=0, | |
) |
Maybe just have a single line
src/mrinufft/operators/autodiff.py
Outdated
[torch.transpose((1j * y[i] * inufft_dx_dom[i]), 0, 1)[None, ...] for i in range(y.shape[0])], | ||
dim=0, | ||
) | ||
grad_traj = torch.mean(grad_traj, dim=0).type_as(traj) |
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.
…ufft into test_autodiff-multicoil
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.
A minor, yet urgent issue to be addressed.
@@ -32,14 +31,12 @@ | |||
(4, 2, True), | |||
], | |||
) | |||
|
|||
@parametrize_with_cases( | |||
"kspace_loc, shape", | |||
cases=[ | |||
CasesTrajectories.case_nyquist_lowmem_radial3D, |
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.
When did we make this as only 3D test? we need to change this back.
…ufft into test_autodiff-multicoil
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.
LGTM, we can merge once green! Great work @alineyyy
@with_numpy_cupy | ||
def data_consistency(self, image_data, obs_data): |
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.
should be @with_numpy
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 will be handled in #140 as we cannot test this right now.
This will solve #131
Some new configurations about the multi-coils have been set.