Add docs and CLI #11
Add docs and CLI #11
Conversation
src/tomo_preprocessing/__init__.py
Outdated
There was a problem hiding this comment.
I needed to add the # noqa: F401 command, because otherwise one of the pre-commit formatting packages would always remove all lines here (because imported functions are not used).
Not sure if there is a more elegant way to avoid this.
There was a problem hiding this comment.
Hmm I think this gets removed because the __all__ exposing these imports is not implemented. In any case, I think it's fine to use the noqa for now.
https://stackoverflow.com/questions/44834/what-does-all-mean-in-python
There was a problem hiding this comment.
This is the Typer CLI for the amplitude matching.
Again, I had to add the command # noqa: B008, because otherwise a pre-commit package (I think ruff) would complain about the "Option" class being used in the function arguments.
There was a problem hiding this comment.
no worries. I think it's fine here.
There was a problem hiding this comment.
Typer CLI for pixel size matching
There was a problem hiding this comment.
This script is new: In the previous version, I only had pixel size matching based on Fourier cropping / extension. However, this does not work for binary segmentations given from the U-Net. So it was not possible to rescale the segmentation to the original tomogram pixel size.
This function does exactly that: It rescales the segmentation to match the original tomogram using scipy's zoom function.
There was a problem hiding this comment.
Hey @LorenzLamm ! Sorry for the delay. I think this looks really nice! I left some minor comments below. @alisterburt and I are going to be out on a bike trip next week, so please feel free to merge yourself once you address the comments.
I am not sure how the github pages is configured for teamtomo, so we may need to update the docs building CI, but let's do that in a follow up PR.
Nice work!
src/tomo_preprocessing/__init__.py
Outdated
There was a problem hiding this comment.
Hmm I think this gets removed because the __all__ exposing these imports is not implemented. In any case, I think it's fine to use the noqa for now.
https://stackoverflow.com/questions/44834/what-does-all-mean-in-python
| @@ -0,0 +1,94 @@ | |||
| # Preprocessing | |||
There was a problem hiding this comment.
no worries. I think it's fine here.
| ----- | ||
| This function uses the FFT algorithms from numpy.fft for performing the Fourier | ||
| transform, and it assumes that the input tomogram has voxel intensities that | ||
| can be converted to floating point numbers. |
There was a problem hiding this comment.
I think it would be helpful to clarify what "voxel intensities that can be converted to floating point numbers" means.
| if (pixel_size_in / pixel_size_out) < 1.0: | ||
| resized_data = fourier_cropping(data, output_shape, smoothing) | ||
| else: | ||
| resized_data = fourier_extend(data, output_shape, smoothing) |
There was a problem hiding this comment.
Is there a need to cover the case where pixel_size_in == pixel_size_out (i.e., just pass the image)?
There was a problem hiding this comment.
Theoretically, nothing should happen in the "fourier_extend" case with the same pixel sizes.
But also doesn't hurt to add this third case to avoid any issue. I added it.
| target_dim / original_dim | ||
| for target_dim, original_dim in zip(output_shape, data.shape) | ||
| ] | ||
| resized_data = ndimage.zoom(data, rescale_factors, order=1) |
There was a problem hiding this comment.
Is the order=1 going to cause an issue? It seems like interpolating a segmentation mask could yield erronous values (e.g., if the users passes a label image or a mask of dtype int, the interpolation would "create" values at boundaries). It might make more sense to use interpolationorder=0. You may also need to set prefilter=False (filtering the mask could also lead to erroneous values).
https://docs.scipy.org/doc/scipy/reference/generated/scipy.ndimage.zoom.html
There was a problem hiding this comment.
Ah, good catch! Yes, order=1 will cause issues I guess. Replacing it with order=0, and also setting prefilter=False, although that only takes an effect when order > 1. But also doesn't hurt.
| The file path to the input tomogram to be processed. | ||
| output_path : str | ||
| The file path where the processed tomogram will be stored. | ||
| pixel_size_in : float |
There was a problem hiding this comment.
I think this is fine for now, but are voxels always isotropic (i.e., does this need to support an array input)?
There was a problem hiding this comment.
Good question!
I have never encountered any tomograms that had anisotropic voxel sizes. I feel the image acquisition schemes and reconstruction algorithms should always lead to isotropic resolution (neglecting e.g. missing wedge artifacts). But maybe there are also exceptions? Need to look into this
Disclaimer for MemBrain being under development Co-authored-by: Kevin Yamauchi <kevin.yamauchi@gmail.com>
Remove mkdocs description. Co-authored-by: Kevin Yamauchi <kevin.yamauchi@gmail.com>
remove redundant comment Co-authored-by: Kevin Yamauchi <kevin.yamauchi@gmail.com>
I added some documentation for the tomo_preprocessing module in the form of better docstrings (tried to have numpy docstring style for all functions), and a Readme file for this module. Also, I used the teamtomo docs structure, where I have an overview page and the preprocessing one so far.
(I guess it should all be prettier in the end with some images & more examples on how to use)
I also tried to implement the Typer CLI (thanks for the suggestion, Alister), which seems really nice to use.
By adding it also to pyproject.toml file, preprocessing methods can now be accessed using
"tomo_preprocessing NAME_OF_PREPROCESSING".
Let me know if this is going in the right direction. If not, I'm happy to adjust -- if yes, I'll extend the documentations and Typer CLIs also to the other functionalities (i.e. training and prediction).