Skip to content

Conversation

@Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Jul 29, 2021

part of #2648 .

Description

This PR enhanced the RandLambdad transform and also added array version RandLambda transform to execute transform for random part of dataset.

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.

@Nic-Ma Nic-Ma mentioned this pull request Jul 29, 2021
7 tasks
Signed-off-by: Nic Ma <nma@nvidia.com>
@rijobro
Copy link
Contributor

rijobro commented Jul 29, 2021

Doesn't this imply that we should just implement RandResize, RandCenterSpatialCropd and ThresholdIntensityd?

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jul 29, 2021

Hi @rijobro ,

The basic idea of this RandCompose is to randomly execute transforms on the data, it's random prob "YES or NO", not like our other random size / random value transforms.
And with this RandCompose, no need to implement the "random" version transform for every deterministic transform anymore.

Thanks.

@wyli
Copy link
Contributor

wyli commented Jul 29, 2021

My question is why this implemented as a special compose method, in the example prob=0.2 for Resized, prob=0.3 for CenterSpatialCropd, and prob0.4 for ThresholdIntensityd are all independent events. Instead, I think each of them could be implemented with a transform wrapper inheriting the RandomizableTransform interface https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/transform.py#L236

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jul 29, 2021

Hi @wyli ,

Thanks for your suggestion.
Actually, at the beginning, I planned to enhance the RandLambdad transform to support prob, but I feel maybe using RandCompose can help simplify a list of wrapper of transforms.
Anyway, if you guys prefer to wrapper, let me change back to enhance the RandLambdad.

Thanks.

@rijobro
Copy link
Contributor

rijobro commented Jul 29, 2021

I think enhancing RandLambda would be more appropriate. Even though most users will combine transforms in a chain, we shouldn't assume that they will all of the time.

Nic-Ma added 2 commits July 30, 2021 11:58
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma force-pushed the 2648-add-randcompose branch from ca11ac6 to 7358326 Compare July 30, 2021 04:03
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma changed the title [WIP] 2648 Add RandCompose to execute deterministic transforms for random part of dataset 2648 Enhance RandLambda to execute deterministic transforms for random part of dataset Jul 30, 2021
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma marked this pull request as ready for review July 30, 2021 04:10
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jul 30, 2021

Hi @rijobro and @wyli ,

I have updated this PR to enhance RandLambdad and also added the array version RandLambda transform.
Could you please help review it again?

Thanks in advance.

@Nic-Ma Nic-Ma requested review from ericspod, rijobro and wyli July 30, 2021 04:12
@wyli
Copy link
Contributor

wyli commented Jul 30, 2021

Thanks, would it be useful to make this invertible?

@wyli
Copy link
Contributor

wyli commented Jul 30, 2021

Was partly discussed here #2004

Nic-Ma added 3 commits July 30, 2021 18:13
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jul 30, 2021

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jul 30, 2021

Hi @wyli @rijobro ,

I added inverse operation to the PR, could you please help review it again?

Thanks in advance.

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jul 30, 2021

/black

@wyli wyli merged commit 607a31a into Project-MONAI:dev Jul 30, 2021
wyli pushed a commit to wyli/MONAI that referenced this pull request Aug 3, 2021
…m part of dataset (Project-MONAI#2667)

* [DLMED] add RandCompose

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] add unit tests

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] change to enhance RandLambda

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] remove RandCompose

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] fix format

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] enhance doc

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] add inverse operation

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] add more tests

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] fix subprogress issue

Signed-off-by: Nic Ma <nma@nvidia.com>
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.

3 participants