-
Notifications
You must be signed in to change notification settings - Fork 2
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
espirit csm #95
base: main
Are you sure you want to change the base?
espirit csm #95
Conversation
efdd7f6
to
4026aa4
Compare
src/mrpro/data/_CsmData.py
Outdated
_, S, VH = torch.linalg.svd(mat, full_matrices=False) | ||
|
||
# Get kernels | ||
kernels = rearrange(VH[S > thresh * S.max()], 'n (coils cba) -> n coils cba') |
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.
Background:
In eq.(9), we need to calculate V_|| that is associated with the row space of the Calibration matrix A. This is later used to construct the forward matrix W. In principle, V_|| is obtained from V by trimming off the row vectors corresponding to zero eigen-values. However, here, they have done it based on this mask: "S > thresh * S.max()". This makes sense as eigen-values may not go to 0 because of the presence of noise.
Problem:
This trimming of VH[S > thresh * S.max()] based on the mask (S > thresh * S.max()) is not supported inside "vmap" operations. I have a feeling that operations like this change the size of the matrix, and that poses challenges to perform batch operations, given that all elements of the batches should have the same shape.
Possible Solution:
If we want to adopt a parallelized approach like before, maybe it makes sense to perform vmap only till the SVD is calculated, since SVD is usually a computationally demanding algorithm for large matrices. Thereafter, we can use loops to construct V_||'s, where different V_||'s can have different shapes based on the batched SVD results. For this, I can use the smap function defined here. However, based on the layout of this file (define _iterative_walsh_csm and then estimate_walsh, define estimate_espirit and then _espirit_csm) , I am not sure if opening up this code to perform first part in one place (SVD calculation using vmap), while performing the other part in another place (forward operation using loops), will break the stye of the code.
If we want to retain the original coding style, I can use the smap function over the whole _espirit_csm function where the SVD is included, but then we would not be parallelizing the SVD.
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 would just switch to fully serial calculation of the csms for different others using smap.
📚 Documentation |
A work-in-progress for espirit algorithm for cartesian images.
This is blocked until we have
Other TODOs/Notes:
The logic is derived from sigpy
we would have to check the license
Why implement it and not wrap sigpy?
Sigpy only works with Cartesian data and is sometimes annoying to install, as it uses cupy for the GPU implementation.
Closes #34