Make transforms optional

[bhashemian]

Description

This PR makes the transform argument in the existing datasets optional (CacheDataset, SmartDataset, etc.). I believe the user should be able to use these datasets without providing a transform.

Status

Ready

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).

[Nic-Ma]

Hi @drbeh ,

Thanks for raising this discussion.
I think we didn't default transform to None in the cache datasets because if you don't have transform, then no need to cache anymore, so no need to use these datasets.
What do you think?
CC @wyli @ericspod @rijobro for more discussion if necessary.

Thanks.

[bhashemian]

Hi @drbeh ,

Thanks for raising this discussion. I think we didn't default transform to None in the cache datasets because if you don't have transform, then no need to cache anymore, so no need to use these datasets. What do you think? CC @wyli @ericspod @rijobro for more discussion if necessary.

Thanks.

Hi @Nic-Ma, thanks for your reply. I see your point but what if the user want to cache the images (without any transform) to save the IO time?

In pathology that loading patched are very costly, one may want to use smart cache dataset even without any transform.

[Nic-Ma]

Hi @drbeh ,

Thanks for your explanation.
If you want to cache the loaded images, usually at least you need a LoadImaged transform instead of None transform.
Could you please help provide some example or draft code to help us understand your use case?

Thanks in advance.

[bhashemian]

@Nic-Ma, PatchWSIDataset, for instance, does not require any transform and extract and load patches from whole slide images.

[Nic-Ma]

@Nic-Ma, PatchWSIDataset, for instance, does not require any transform and extract and load patches from whole slide images.

Hi @drbeh ,

I think we already set transform=None as default for PatchWSIDataset:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/wsi_datasets.py#L58
And it's the same as its base class:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/dataset.py#L65
I mean we usually have transforms for cache datasets, not all kinds of datasets, or maybe I misunderstood your use case?

Thanks.

[bhashemian]

@Nic-Ma, PatchWSIDataset, for instance, does not require any transform and extract and load patches from whole slide images.

Hi @drbeh ,

I think we already set transform=None as default for PatchWSIDataset: https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/wsi_datasets.py#L58 And it's the same as its base class: https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/dataset.py#L65 I mean we usually have transforms for cache datasets, not all kinds of datasets, or maybe I misunderstood your use case?

Thanks.

I'm trying to give you an idea where it is possible to need caching data without having a transform. Maybe it's easier to discuss it online.

[Nic-Ma]

After some discussion, here are my final comments:

1. We can add transforms=None to CacheDataset and its subclasses, for the use case to cache data of datasets chain:

data = WSIPatchDataset(..., transform=None)
cachedata = SmartCacheDataset(data=data, transform=None, replace_rate=0.2)

2. But no need to add transform=None to PersistentDataset, because PersistentDataset must start from file path and compute hash value with it.
3. Please add more unit tests to cover the transforms=None in these datasets.
4. And please also add the missing unit test case you said in: Disable pylint error and fix CI tests of new tifffile #4162 (comment)

@wyli @ericspod Do you have any other opinions?

Thanks in advance.

[Nic-Ma]

Hi @drbeh ,

And I think maybe it's better to add None support for the transform arg but doesn't set the default value to None, because for most cases we expect transforms, refer to:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/dataset.py#L204

What do you think?

Thanks.

[bhashemian]

We can do that but it's the same for Dataset where we set default value to None:

MONAI/monai/data/dataset.py

Line 65 in 05b9d31

def __init__(self, data: Sequence, transform: Optional[Callable] = None) -> None:

Isn't better to have the same thing here?

[bhashemian]

@wyli @Nic-Ma any other comments here?

[wyli]

Thanks, it looks good to me. I'll merge this once this is fixed #4200

[wyli]

/build