Skip to content

2648 Enhance RandLambda to execute deterministic transforms for random part of dataset #2667

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jul 30, 2021
Merged

2648 Enhance RandLambda to execute deterministic transforms for random part of dataset #2667

merged 14 commits into from
Jul 30, 2021

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Jul 29, 2021 •
edited
Loading

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
Merged
7 tasks
@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 •
edited
Loading

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 •
edited
Loading

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 Nic-Ma force-pushed the 2648-add-randcompose branch from ca11ac6 to 7358326 Compare July 30, 2021 04:03
@Nic-Ma
[DLMED] fix format
69090c4 
Signed-off-by: Nic Ma <[email protected]>
@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
@Nic-Ma
[DLMED] enhance doc
a6b5d18 
Signed-off-by: Nic Ma <[email protected]>
@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 wyli, rijobro and ericspod 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
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.

wyli
wyli reviewed Jul 30, 2021
View reviewed changes
@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
@Nic-Ma @wyli
2648 Enhance RandLambda to execute deterministic transforms for rando…
5d56fd5 
…m part of dataset (Project-MONAI#2667)

* [DLMED] add RandCompose

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add unit tests

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] change to enhance RandLambda

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] remove RandCompose

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix format

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] enhance doc

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add inverse operation

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] add more tests

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix subprogress issue

Signed-off-by: Nic Ma <[email protected]>
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

@rijobro rijobro Awaiting requested review from rijobro

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

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
@Nic-Ma @rijobro @wyli