Deconvolution as a preprocessing option

[rdrighetto]

Hi,

I implemented the deconvolution filter from Warp (a.k.a. "dimifilter" or "tom_deconv") as an option in tomo_preprocessing.

The implementation is based off https://github.com/C-CINA/focustools. While that implementation used numexpr to accelerate the large array calculations, here I switched to pure numpy to avoid adding one more dependency to membrain_seg. However, I feel the performance could certainly be improved by incorporating something like numexpr, numba, or maybe even torch (on GPU)? Suggestions are very welcome.
It takes ~4 minutes on my AMD EPYC CPU to deconvolve a 928x928x464 tomogram, but a tomogram that is only slightly bigger (1024x1024x512) already takes ~8 minutes.

The code passes the pre-commit tests, but if any further changes are required please let me know.

[LorenzLamm]

Thanks for the PR, Ricardo! :)

The functionalities look good to me and the filtering performance is really nice. However, as mentioned in the comments also, I'm wondering whether it may be more elegant to use the focustools package as dependency and implement the filter based on this?
This may be easier maintenance-wise, and I also assume that the numexpr dependency is not so bad, especially if it improves the efficiency.

Let me know what you think

[LorenzLamm]

Suggested change

	`tomo_preprocessing deconvolve --input <path-to-tomo> --output <path-to-output-tomo> --df1 <defocus-of-the-zero-tilt>`
	`tomo_preprocessing deconvolve --input <path-to-tomo> --output <path-to-output-tomo>

[LorenzLamm]

Maybe we can remove the df1 command from this example to show how easy it is is normally to use the filter.
Or is it recommended to use the df1 argument most of the time? In this case, should we make it a required argument by the CLI?
(currently, only input and output files are required arguments)

[rdrighetto]

No, --df1 is the most important option of the deconvolution and the correct value will be different for every tomogram. Actually I should make this option mandatory instead of just having a default value, to force the user to think about what they are doing.

[rdrighetto]

The --df option is now mandatory (prompted for if not specified). It is now simplified, only a single defocus value (and hence no astigmatism) is requested from the user.

[LorenzLamm]

Would it make sense to add the focustools package as a dependency and build the functionality around that?

Like in a previous PR (https://github.com/teamtomo/membrain-seg/pull/21/files), it may be better to leave implementations like this to other packages, s.t. we don't have to take care of the maintenance of this functionality as well (i.e., you would have to maintain it in the focustools package, Ricardo ;) )

[rdrighetto]

Thanks for the suggestion, but I would prefer to not have focustools as a dependency for membrain-seg. It is an old, semi-abandoned package that I don't consider mature enough for membrain-seg to rely on it. Conversely, I don't think it would make sense to maintain it just because membrain-seg wants to use it.
Since other preprocessing options in the Fourier world are already implemented natively (pixel size matching and spectral matching), I thought it was natural for membrain-seg to have its own deconvolution module. In the future we could even try things like improve the deconvolution model to enhance segmentation performance.

[LorenzLamm]

Ah okay, just saw that you also updated with the 3D implementation in the focustools package, and thought you might maintain it regularly.
Then I agree -- let's keep it in here for now, similar to the other Fourier preprocessing.
Maybe it makes sense for the future to outsource preprocessing methods into a separate package? I think this could be useful not only for membrane segmentation, but for many kinds of processing.

[rdrighetto]

Yes, if we follow this argument for focustools, then we should also use it for the spectral matching and the pixel size matching, as they are all implemented there. But I think in membrain-seg they are more elegant and I agree that in the future they can evolve into a separate preprocessing package.

[rdrighetto]

Regarding focustools, I don't think including it as a dependency is a good idea (please see detailed comment above).

As for numexpr, that yes is a mature package that I consider to be a low-risk dependency. Let me just do a few more tests with it to see if the performance gains are really significant here.

[rdrighetto]

Unfortunately, numexpr did not give meaningful performance gains over pure numpy. I might not be using it the right way, or tweaking its options is needed to obtain good speedups. This is probably overkill, so will stick to pure numpy for the time being.

[LorenzLamm]

Looks great :)
Maybe, in a follow-up, we could rename some variables (like "S") to more meaningful names to improve readability for the code. But for now, I think we can merge