search

nion-software / eels-analysis

A Nion Swift package for doing EELS analysis.
GNU General Public License v3.0
6 stars 9 forks source link

Changed shift method in ZLP subpixel align to scipy.ndimage.shift wit… #29

Closed Brow71189 closed 5 years ago

Brow71189 commented 5 years ago

…h linear interpolation to get rid of artifacts.

Matt showed me a dataset that had some artifacts in it after using subpixel ZLP align (see here: https://drive.google.com/file/d/1vIZJBwQRZIoZhSbFn_wwN7BzqfSZ8YIX/view?usp=sharing). This periodic pattern that is on top of the data comes from the scipy.ndimage.fftshift method. When using scipy.ndimage.shift with interpolation oder 1 (i.e. linear interpolation) the artifacts are gone. I don't think this interpolation method has any negative effects, so it should be safe to merge.

cmeyer commented 5 years ago

FFT shift was chosen to minimize edge artifacts, keep statistics reasonable, and to be more precise -- so this change may solve one case only to make another case worse.

For the referenced image, can you include the input data and describe how the artifacts arise?

What about test cases? (see next paragraph)

1D (and 2D) shifting is provided by Core.py. Can we spend the time implementing/testing the algorithm there and then use that version instead of reimplementing in eels_analysis?

(bigger picture: do we need to allow users to choose algorithm details such as what type of alignment to use?)

Brow71189 commented 5 years ago

I agree that we should use the shift function from Core.py. However I can't really see the benefit of fftshift: fftshift wraps edges around the image, which is something you can also tell the other shift functions to do. And how exactly is fftshift more precise than real space shift with appropriate interpolation? You get the artifacts when you align the raw data with Align ZLP (com) or Align ZLP (peak fit) and the nintegrate the sequence. Here is the raw data: https://drive.google.com/file/d/1DGGXTso6NNTbyEib9PNNdBtPreXUcqrr/view?usp=sharing Changing the shift method gets rid of them, so does Align ZLP (max) which is directly doing integer pixel shifts.

cmeyer commented 5 years ago

"Allow choice of Fourier shift or regular shift in sequence_align functions " https://github.com/nion-software/niondata/issues/9

Brow71189 commented 5 years ago

If that means that we want to give the user the option to choose the shift method then we should probably do the same for AlignZLP since it is a very similar operation

cmeyer commented 5 years ago

Yes, but then the menu might not be sufficient. There will be 3 algorithm varieties (max, com, peak fit) and 2 sub varieties (fourier shift or linear shift) and possibly other variations (edge handling, for instance).

So in this case, maybe linear shift with sensible edge handling is just the default for now (as submitted in this patch) until we have some UI mechanism to allow the user to more easily specify variations (yes, I know we could do it in the inspector, but that all needs an overhaul before going too much further with it).

How are the edges handled in scipy.shift?

Brow71189 commented 5 years ago

Sounds good. Right now the edges are set to "constant" (default) which will fill with zeros by default. But you can choose between different options like wrap, mirror etc

Chris Meyer notifications@github.com schrieb am Do., 3. Okt. 2019, 19:34:

Yes, but then the menu might not be sufficient. There will be 3 algorithm varieties (max, com, peak fit) and 2 sub varieties (fourier shift or linear shift) and possibly other variations (edge handling, for instance).

So in this case, maybe linear shift with sensible edge handling is just the default for now (as submitted in this patch) until we have some UI mechanism to allow the user to more easily specify variations (yes, I know we could do it in the inspector, but that all needs an overhaul before going too much further with it).

How are the edges handled in scipy.shift?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nion-software/eels-analysis/pull/29?email_source=notifications&email_token=AD7SKK3MGVUKZLJSQHLN6S3QMYUJLA5CNFSM4I4UIADKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAI7D3I#issuecomment-538046957, or mute the thread https://github.com/notifications/unsubscribe-auth/AD7SKK3HA2TR6DOCQUA5HKTQMYUJLANCNFSM4I4UIADA .

cmeyer commented 5 years ago

Merged. Minor point: HyperSpy uses interpolate.interp1d.