EIDOSLAB / torchstain

Stain normalization tools for histological analysis and computational pathology
MIT License
112 stars 20 forks source link

Multiple backends support #11

Closed andreped closed 2 years ago

andreped commented 2 years ago

As we have implementations of the Macenko algorithm for torch, tf, and numpy backends, it would be a good idea to not have everything into a single binary, as it would mean that the user would need to install both tf and torch to use torchstain, which often collides with underlying deps.

As discussed here, a good solution would be to make it possible to: pip install torchstain[backend]

where one would select whether to install torchstain with tf or torch backends. Numpy can be included in both.

My idea is to make submodules within the torchstain module, and then use the extra_require option in setuptools to install specific submodules. I have started the implementation in a separate branch: https://github.com/andreped/torchstain/tree/backends/torchstain

Some pieces remain, but when this works properly I believe we are ready to make a new release.

carloalbertobarbano commented 2 years ago

The idea of having separate modules as in your branch is good, as we also need to port the different utilities from pytorch to tf. I tried to setup a project (you have write access to it) so that we can keep track of all the features

andreped commented 2 years ago

I've gotten a bit further now. It is not finished yet, but do you approve of the design, @carloalbertobarbano? https://github.com/andreped/torchstain/tree/backends/torchstain

Need to get the pip install torchstain[backend] working somehow as well. As well as, verifying that you are able to install torchstain with a specific backend, without needing the other; if you install with torch, you do not install tf at all, and vice versa.

However, there are some duplicate code between the subpackages, so there are probably improvements to be made.

carloalbertobarbano commented 2 years ago

Yes I think it's on the right track. Probably the base classes such as HENormalizeer, MacenkoNormalizer etc should be common to both modules, maybe in a base module.

andreped commented 2 years ago

Good idea. The problem is that when I install, I wish to install some specific subpackage, without the other, so it was easier to include the base both places, but I agree. Having a base subpackage is more scalable.

carloalbertobarbano commented 2 years ago

I think the entire torchstain library could be installed, and then the user will be able to specify backend="torch" or backend="tensorflow" when instantiating a normalizer.

It might event be useful to have a global method such as torchstain.set_backend() to set a default backend instead.

The imports in torchstain should be done based on the backend specified by the user (by doing "pip install torchstain[tf]" the user is just saying that the pytorch dependency is not necessary and won't be installed).

andreped commented 2 years ago

Ok, I think I managed to get it properly working now. It was a lot more annoying to do than first anticipated, due to packages/modules, dependencies, and imports operate in Python. At least all the CI tests works fine, and you can use the tool with torch without needing tensorflow, and vice versa. Same usage as before. Just choose which backend to use to the MacenkoNormalizer class. I also made a separate base subpackage as discussed.

The last thing that remains is to be able to simplify the imports, which I believe you might have an idea of how to do. I didn't find a simple way, probably some simple __init__.py magic needed :P

Currently, for using the MacenkoNormalizer, you need to run: torchstain.base.normalizers.macenko_normalizer.MacenkoNormalizer(), but ideally you should just run: torchstain.MacenkoNormalizer().

For debugging, please, see my current implementation: https://github.com/andreped/torchstain/tree/backends/torchstain

I will make a PR and you can make comments and reviews there.

carloalbertobarbano commented 2 years ago

Resolved in https://github.com/EIDOSLAB/torchstain/pull/16