Skip to content

Pathology Masked Inference WSI Dataset#1869

Merged
bhashemian merged 31 commits intoProject-MONAI:masterfrom
bhashemian:pathology_inference
Mar 31, 2021
Merged

Pathology Masked Inference WSI Dataset#1869
bhashemian merged 31 commits intoProject-MONAI:masterfrom
bhashemian:pathology_inference

Conversation

@bhashemian
Copy link
Copy Markdown
Member

@bhashemian bhashemian commented Mar 27, 2021

Description

This PR implements a dataset for pathology inference. It uses tissue foreground masks to extract patches from whole slide images and provide necessary meta data for post-processing of inference results.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@bhashemian bhashemian requested review from Nic-Ma and wyli March 27, 2021 01:59
@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 27, 2021

@wyli @Nic-Ma , it's ready for review. Thanks.

@bhashemian bhashemian enabled auto-merge (squash) March 28, 2021 04:20
Nic-Ma
Nic-Ma reviewed Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put minor comments.
Others look good to me.
Thanks.

@bhashemian
Remove prints
6f9edbe 
Signed-off-by: Behrooz <[email protected]>
@Nic-Ma Nic-Ma disabled auto-merge March 29, 2021 05:37
wyli
wyli requested changes Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I put some comments inline, mainly asking for some docstring/variable name updates

monai/apps/pathology/datasets.py Outdated

class MaskedInferenceWSIDataset(Dataset):
"""
This dataset load the provided tissue masks at an arbitrary resolution level,
Copy link
Copy Markdown
Contributor

@wyli wyli Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for clarification, could you revise the terminology in this file, there are data, sample, patch, region, sub_region. what's the difference between mask_location and image_location?

Copy link
Copy Markdown
Member Author

@bhashemian bhashemian Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sample is one of the data. We first extract region from a WSI, then extract multiple patches from that region based on the grid_shape. I'll remove sub_region.

mask and image have different resolution level, so their location, although related, are not the same.

Copy link
Copy Markdown
Contributor

@wyli wyli Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, what are the differences in data, dataset, WSI? could you please document these, so that it'll be easier to maintain for the other developers

Copy link
Copy Markdown
Member Author

@bhashemian bhashemian Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wyli, I modified a bit the naming. Now we have self.data like other Datasets, which is a collection of samples, and we have the hierarchy of data > sample > patch.

WSI is whole slide image and we don't have a variable specifically called WSI, do we?!
It is only represented in the class names and variables like wsi_object_dict, which is a dictionary of either OpenSlide or CuCIM objects.

And we don't have dataset variable here but I general refer to an instance of Dataset as dataset.

Copy link
Copy Markdown
Contributor

@wyli wyli Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the docstring. it's also about explaining this in the code, to help the other developers. "sample is one of the data. We first extract region from a WSI" is already confusing...

Copy link
Copy Markdown
Member Author

@bhashemian bhashemian Mar 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In MONAI's Dataset data is Sequence of something, what do you call it? I called it here sample.

Extracting a region from a whole slide image is very common in digital pathology, what part of it shoud I explane more?

Copy link
Copy Markdown
Contributor

@wyli wyli Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you put a paper reference here? if that could help clarify the logic. Perhaps we could have comments from Ziyue?

@bhashemian bhashemian enabled auto-merge (squash) March 30, 2021 13:56
@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 30, 2021

@wyli, could you please check if all your comments are addressed? Have I missed anything?

wyli
wyli reviewed Mar 30, 2021
View reviewed changes
@bhashemian
Remove save_dir
ea0cf42 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Update docstring
516f52a 
Signed-off-by: Behrooz <[email protected]>
wyli
wyli approved these changes Mar 30, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! sorry I could have suggested this earlier, perhaps code + a paper reference, everything becomes self-contained

@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 30, 2021

thanks! sorry I could have suggested this earlier, perhaps code + a paper reference, everything becomes self-contained

@wyli, no problem, just could you please let me know which part of the code you think a paper reference would be appropriate?

@bhashemian bhashemian mentioned this pull request Mar 30, 2021
Merged
7 tasks
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 30, 2021 •
edited
Loading

thanks! sorry I could have suggested this earlier, perhaps code + a paper reference, everything becomes self-contained

@wyli, no problem, just could you please let me know which part of the code you think a paper reference would be appropriate?

the concept of sampling data into region/grid/patch representations is used throughout this module, would be nice if we could cite any of the existing papers that uses this topology

@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 30, 2021

thanks! sorry I could have suggested this earlier, perhaps code + a paper reference, everything becomes self-contained

@wyli, no problem, just could you please let me know which part of the code you think a paper reference would be appropriate?

the concept of sampling data into region/grid/patch representations is used throughout this module, would be nice if we could cite any of the existing papers that uses this topology

That makes sense! I'll find a paper and will add it.

@bhashemian bhashemian merged commit 8010e2a into Project-MONAI:master Mar 31, 2021
@bhashemian bhashemian deleted the pathology_inference branch April 12, 2021 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

@Nic-Ma Nic-Ma Nic-Ma approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bhashemian @wyli @Nic-Ma