Cdi recon #279
Cdi recon #279
Conversation
|
This should be ready to review. |
| @@ -96,6 +98,27 @@ def gauss(dims, sigma): | |||
| return y / np.sum(y) | |||
|
|
|||
|
|
|||
| def _fft_helper(x, option='fft'): | |||
There was a problem hiding this comment.
I would suggest two functions _fft_helper and _ifft_helper. Switching on a string like this is not a great pattern. If you really want to be able to switch based on a string key, I would implement this as a dictionary look up of lambda functions. This will fail in really surprising ways if option is not 'fft' or 'ifft'.
There was a problem hiding this comment.
That is actually my original thought. However, the two helper functions are so similar, so I summarized them as one. If you strongly suggest that way, sure.
There was a problem hiding this comment.
If these are for internal use only I would just do
_fft_normed = lambda x: np.fft.ifftshift(np.fft.fftn(np.fft.fftshift(x)))
_ifft_normed = lambda x: np.fft.ifftshift(np.fft.ifftn(np.fft.fftshift(x)))Wouldn't it be simpler to just leave the fft in the natural order and only shift it when needed?
There was a problem hiding this comment.
also, why is fftshift being called on real space data on its way in/out?
There was a problem hiding this comment.
I also would not call this 'normed' with the amplitude being scaled by something.
There was a problem hiding this comment.
I would just call it _fft_helper or _ifft_helper
| """ | ||
| sup = np.zeros(shape_v) | ||
| dummy = _dist(shape_v) | ||
| sup_index = np.where(dummy <= sup_radius) |
There was a problem hiding this comment.
sup[dummy < sup_radius] = 1
The pattern in python leans towards open rather than closed intervals.
|
@tacaswell , any more comments on the update? |
|
Back on my list of things to re-read. |
| reconstructed pattern in real space | ||
| diffracted_pattern : array | ||
| diffraction pattern from experiments | ||
| offset_v : float |
|
Corrections are made except fft/ifft part. As fft_helper is shown in multiple places, it is better to be a bit cautious when we do the updates. |
|
I don't think you addressed why cdi has it's own version of _dist when I think there is a function in |
The full version of cdi reconstruction is added.