Skip to content

Conversation

@bhashemian
Copy link
Member

@bhashemian bhashemian commented May 10, 2022

Fixes #4247

Description

This PR add split patch functionality to PatchWSIReader.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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 --disttests.
  • In-line docstrings updated.

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@bhashemian bhashemian requested review from Nic-Ma and wyli May 10, 2022 13:05
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 10, 2022

Thanks for the implementation of new feature request.
I am wondering if it's possible to support the split patch functionality in a transform, which handles the output of PatchWSIDataset with other transforms?
@wyli @ericspod What do you think?

Thanks.

@wyli
Copy link
Contributor

wyli commented May 10, 2022

I lose the overall picture of the WSI-related reader/dataset modules, would be great to have some tutorials in https://github.com/Project-MONAI/tutorials/tree/master/modules to demonstrate the use cases, otherwise it's not easy to help review these PRs.

@ericspod
Copy link
Member

Thanks for the implementation of new feature request. I am wondering if it's possible to support the split patch functionality in a transform, which handles the output of PatchWSIDataset with other transforms? @wyli @ericspod What do you think?

Thanks.

Typically we would want to decouple behaviours so that we have the option to choose what to use, and reuse functionality elsewhere. Using GridSplit here as a hard-coded transform is something I would guess is done as an efficiency necessity which we had discussed in previous PRs, or just such common practice that it makes sense to have a built-in option. @drbeh correct me if I'm wrong!

@wyli
Copy link
Contributor

wyli commented May 10, 2022

(For an example of the tutorials: this PR proposes code changes #4236, and the corresponding usage is shown in a PR to the tutorials Project-MONAI/tutorials#690. These greatly help the reviewing/discussion process...)

@bhashemian
Copy link
Member Author

@Nic-Ma @ericspod, this split does not have any randomness (like what we have discussed before) and I had in mind to use the transform only but the reason to have the split here is to managing the following transforms. What I mean is that the split transform generate several patches (from few patches to few hundreds or thousands of patches), and we want these patches treated independently like images in a batch. It was not clear to me how its output will be handle by its following transforms. Assume the following:

  • input patch: (3, 16, 16)
  • (3, 20, 24) -> GridSplit (2x2) -> (4, 3, 5, 6) [mind the additional dimension]
  • (4, 3, 5, 6) -> OTHER TRANSFORMS -> (4, 3, 5, 6) [does all other transform acknowledge the batch dimension?]
  • (4, 3, 5, 6) -> BATCHING (10) -> (40, 3, 5, 6) [not (10, 4, 3, 5, 6), can we achieve this without a new collate function?]

I would really like to make this as a separate transform so I'd appreciate it if you have any solution that can create the batches as we like (without implementing a new collate function).

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 10, 2022

Hi @drbeh ,

I think your problem is similar to our RandCropByPosNegLabeld transform which generates a list of patches:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/croppad/dictionary.py#L1042
The following transforms will apply for each item of the patch list:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/transform.py#L88

Thanks.

@bhashemian
Copy link
Member Author

https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/transform.py#L88

@Nic-Ma, this is what the current implementation is already leveraging and that's why I put GridSplit in the dataset so the transform chain can be applied on each of them.

However, this is not the case if a transform at the middle of chain creates a list of images, right?

@bhashemian
Copy link
Member Author

I think your problem is similar to our RandCropByPosNegLabeld transform which generates a list of patches:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/croppad/dictionary.py#L1042

I think the problem is similar, so how this is being batched? Is there any issue for the other transforms if it is a list?

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 10, 2022

RandCropByPosNegLabeld can be in the middle of a transform chain, and we also have several other transforms generate a list of samples.
monai.data.DataLoader can batch for this case with the built-in collate function.

Thanks.

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@bhashemian
Copy link
Member Author

@Nic-Ma @ericspod, I have updated the GridSplit and now the transform and dataset are completely separated. Thanks for your inputs.

@wyli, I have updated the tumor detection pipeline that uses these transforms and dataset: Project-MONAI/tutorials#697

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
Nic-Ma
Nic-Ma previously approved these changes May 12, 2022
Copy link
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.

Thanks for the update.
Looks good to me except for some minor comments inline.
And I would suggest to add description for the return data shape in the doc-string.
@wyli @ericspod do you have any other comments?

Thanks.

@wyli
Copy link
Contributor

wyli commented May 12, 2022

@wyli, I have updated the tumor detection pipeline that uses these transforms and dataset: Project-MONAI/tutorials#697

thanks, @Nic-Ma could you please help run and verify the tutorial (perhaps with a subset of CAMELYON16 for a quick setup)?

@bhashemian
Copy link
Member Author

@wyli, I have updated the tumor detection pipeline that uses these transforms and dataset: Project-MONAI/tutorials#697

thanks, @Nic-Ma could you please help run and verify the tutorial (perhaps with a subset of CAMELYON16 for a quick setup)?

That's a good idea. I'll add a subset dataset to the PR of the tutorial to facilitate the testing but we can remove it before merge.

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@bhashemian
Copy link
Member Author

@Nic-Ma @wyli, I have added a subset dataset ('training.txt') into this tutorial PR: Project-MONAI/tutorials#697

Please let me know if you have any question.

@Nic-Ma Nic-Ma dismissed their stale review May 12, 2022 17:03

Minor comment in the code.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 12, 2022

Hi @drbeh @wyli ,

I tried to run the tutorial, seems it can run normally:
image

Thanks.

@bhashemian bhashemian enabled auto-merge (squash) May 12, 2022 17:06
@bhashemian bhashemian disabled auto-merge May 12, 2022 17:09
Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@bhashemian bhashemian enabled auto-merge (squash) May 12, 2022 17:18
@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 13, 2022

/build

@bhashemian bhashemian merged commit 1dfeb74 into Project-MONAI:dev May 13, 2022
@bhashemian bhashemian deleted the split_wsi_dataset branch May 13, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split WSI regions into sub-patches without new dimension

4 participants