Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign of WSIReader #4071

Closed
drbeh opened this issue Apr 4, 2022 · 7 comments · Fixed by #4107 or #4147
Closed

Redesign of WSIReader #4071

drbeh opened this issue Apr 4, 2022 · 7 comments · Fixed by #4107 or #4147
Assignees
Labels
enhancement New feature or request

Comments

@drbeh
Copy link
Member

drbeh commented Apr 4, 2022

Right now WSIReader is based on ImageReader class with read and get_data methods. However, reading whole slide images regardless of the library to use have its own additional complexities that are not captured in the base class.
Also, currently, we have implemented different backends cuCIM, OpenSlide, and TiffFile` through if statements!

My suggestion would be to have a WSIReader base class that implements those additional functionalities such as evaluating the level based on ppm/resolution, validating the pyramidal levels, etc. along with abstract methods that each of the libraries has to implement. In this way, we can implement each backend in different classes like CuCIMReader (for cuCIM) inherited from WSIReader.

This not only makes the code cleaner and more readable but also decouples MONAI integration of third-party WSI reader libraries and makes the future contribution easier and safer.

The overall structure would look like this: (will update #4005)
wsi

@drbeh
Copy link
Member Author

drbeh commented Apr 4, 2022

@wyli @Nic-Ma @ericspod @rijobro
Do you have any comment here? does it make sense to you?

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Apr 5, 2022

Hi @drbeh ,

Thanks for the refactoring idea.
I think it overall looks good to me, maybe we can simplify it to a flattened file structure:
monai/data/wsireader.py and monai/data/wsidataset.py
What do you think?
CC @wyli @ericspod

Thanks.

@ericspod
Copy link
Member

ericspod commented Apr 5, 2022

I think this is a good change though WSIReader should still inherit from ImageReader so that the API remains compatible. TiffFileReader isn't just reading TIFF images but specifically those storing slide data so this class should have a more specific name to not appear to be a general purpose TIFF image reader, PILReader is meant for that instead.

@drbeh
Copy link
Member Author

drbeh commented Apr 5, 2022

Hi @drbeh ,

Thanks for the refactoring idea. I think it overall looks good to me, maybe we can simplify it to a flattened file structure: monai/data/wsireader.py and monai/data/wsidataset.py What do you think? CC @wyli @ericspod

Thanks.

Hi @Nic-Ma, thanks for your feedback. I agree with this naming too.
I just wanted to follow the previous suggestions with @wyli (#3868 (comment)) and @ericspod (#3868 (comment)).
Either way is fine with me, the benefit that I can see for having a wsi directory is to declutter data directory, and add all WSI related stuff, e.g. datasets.py, reader.py, writer.py (maybe in the future), under the same name space (monai.data.wsi).

What do you think? @wyli @ericspod @Nic-Ma

@drbeh
Copy link
Member Author

drbeh commented Apr 5, 2022

I think this is a good change though WSIReader should still inherit from ImageReader so that the API remains compatible. TiffFileReader isn't just reading TIFF images but specifically those storing slide data so this class should have a more specific name to not appear to be a general purpose TIFF image reader, PILReader is meant for that instead.

You are right! Here we are implementing a specific use of tifffile library, so we should be more clear. I'll think of a better name for that.

@wyli
Copy link
Contributor

wyli commented Apr 5, 2022

It looks good, given that monai.data doesn't currently have subfolders, I suspect monai/data/wsireader.py and monai/data/wsidataset.py are more consistent with the rest of the module.

@drbeh
Copy link
Member Author

drbeh commented Apr 5, 2022

Sure, that's fine with me. Follow the same format as image_*.py, I'll make wsi_*.py.

@drbeh drbeh mentioned this issue Apr 19, 2022
7 tasks
Repository owner moved this from Todo to Done in AI in Pathology🔬 Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 💯 Complete
Development

Successfully merging a pull request may close this issue.

4 participants