Skip to content
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.

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

Add SomeOf transform composer #6143

Merged
merged 9 commits into from
Mar 24, 2023

Conversation

tuanchien
Copy link
Contributor

SomeOf transform composer

Description

Adds a new transform composer allowing the user to sample "some of" the transforms from a given list of transforms. Allows sampling a fixed number, or variable number of transforms within a configurable range.
Sampling is uniform and without replacement.

The Compose class does almost all of the heavy lifting. This class just replaces the list of transforms each time with a newly sampled list of transforms on each __call__ invocation. The simple sampling entry point will allow for easy extension later in case there's a need to add support for other probability distributions.

This allows users to compose MONAI transforms in a non sequential way without needing to write their own compositor, or wrapping compositors from frameworks like Albumentations.

Maybe of interest to Issue #5312

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 --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

tests/test_some_of.py Outdated Show resolved Hide resolved
@wyli wyli requested a review from KumoLiu March 14, 2023 10:27
Copy link
Contributor

@KumoLiu KumoLiu 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 your contribution! Just leave some comments in line.

monai/transforms/compose.py Outdated Show resolved Hide resolved
monai/transforms/compose.py Outdated Show resolved Hide resolved
monai/transforms/compose.py Outdated Show resolved Hide resolved
monai/transforms/compose.py Outdated Show resolved Hide resolved
tests/test_some_of.py Show resolved Hide resolved
@ericspod
Copy link
Member

@atbenmurray Is there anything we need to look at here relating to lazy transforms and/or random state?

@ericspod ericspod requested a review from atbenmurray March 14, 2023 16:41
@tuanchien tuanchien force-pushed the feature/someof_composer branch 2 times, most recently from ce03892 to f682caf Compare March 15, 2023 09:29
@atbenmurray
Copy link
Contributor

I'm taking a look now

Copy link
Contributor

@atbenmurray atbenmurray left a comment

Choose a reason for hiding this comment

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

In general, implementations cannot dynamically modify transform lists on a call by call basis without breaking in the general case. OneOf and RandomOrder are good examples of how to implement such a function in a way that the transform list remains static.

tests/test_some_of.py Outdated Show resolved Hide resolved
monai/transforms/compose.py Outdated Show resolved Hide resolved
monai/transforms/compose.py Show resolved Hide resolved
@atbenmurray
Copy link
Contributor

atbenmurray commented Mar 15, 2023

@tuanchien I realise I have dropped a lot of complexity on you here about all this. I am happy to discuss how you might achieve what you want with this PR using a OneOf/RandomOrder style approach. It should be quite a straightforward thing to do.

@tuanchien
Copy link
Contributor Author

@atbenmurray thanks for your offer. Will take a stab at it first.

@tuanchien
Copy link
Contributor Author

tuanchien commented Mar 16, 2023

From my understanding, the differences between this implementation and the Albumentations version are:

This version:

  • Uniformly samples transforms to use.
  • Allowed for varying number of sampled transforms per call.

Albumentations:

  • Takes the p's (probabilities) from the transforms, and normalises them to 1, and uses this as the sampling probability distribution.

@tuanchien tuanchien force-pushed the feature/someof_composer branch from 58f2dfb to 8d3e5dd Compare March 16, 2023 09:48
@tuanchien
Copy link
Contributor Author

tuanchien commented Mar 16, 2023

I've added a weights parameter with a similar normalisation to OneOf to use for sampling. So now it should have feature parity with Albumentations too.

Copy link
Contributor

@atbenmurray atbenmurray left a comment

Choose a reason for hiding this comment

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

Looking much better; a few edge cases in testing still to hit, but nearly there

monai/transforms/compose.py Outdated Show resolved Hide resolved
monai/transforms/compose.py Show resolved Hide resolved
tests/test_some_of.py Show resolved Hide resolved
@tuanchien tuanchien force-pushed the feature/someof_composer branch 4 times, most recently from 00c4e08 to ad419ef Compare March 17, 2023 07:56
@tuanchien tuanchien force-pushed the feature/someof_composer branch from 91e86b2 to e84ee42 Compare March 17, 2023 07:59
Signed-off-by: Tuan Chien <[email protected]>
Signed-off-by: Tuan Chien <[email protected]>
Signed-off-by: Tuan Chien <[email protected]>
Signed-off-by: Tuan Chien <[email protected]>
Signed-off-by: Tuan Chien <[email protected]>
@wyli
Copy link
Contributor

wyli commented Mar 21, 2023

hi @atbenmurray could you please finalize the review here?

@atbenmurray
Copy link
Contributor

Taking a look now

@wyli
Copy link
Contributor

wyli commented Mar 23, 2023

/build

@atbenmurray
Copy link
Contributor

@wyli @Nic-Ma @ericspod Is there an issue here with batches and differing numbers of transforms per-sample?

@wyli
Copy link
Contributor

wyli commented Mar 24, 2023

@wyli @Nic-Ma @ericspod Is there an issue here with batches and differing numbers of transforms per-sample?

good point, I think it should work by looking at the collate function:

MONAI/monai/data/utils.py

Lines 429 to 438 in b87375f

def collate_meta_tensor(batch):
"""collate a sequence of meta tensor sequences/dictionaries into
a single batched metatensor or a dictionary of batched metatensor"""
if not isinstance(batch, Sequence):
raise NotImplementedError()
elem_0 = first(batch)
if isinstance(elem_0, MetaObj):
collated = default_collate(batch)
collated.meta = default_collate([i.meta or TraceKeys.NONE for i in batch])
collated.applied_operations = [i.applied_operations or TraceKeys.NONE for i in batch]

@tuanchien have you tried this transform in an end-to-end training example, with monai's dataloader and dataset?

and @atbenmurray perhaps we can merge this PR now? I think this SomeOf is a good use case, if the collate function doesn't support it, we should improve the collate function...

@wyli
Copy link
Contributor

wyli commented Mar 24, 2023

/build

@wyli wyli enabled auto-merge (squash) March 24, 2023 11:07
@wyli wyli merged commit 4ed5532 into Project-MONAI:dev Mar 24, 2023
@tuanchien
Copy link
Contributor Author

tuanchien commented Mar 25, 2023

@wyli sorry for the late response. I have not looked at the collate function. But for what it's worth, the project I used SomeOf on was end-to-end using MONAI transforms, data loaders, and dataset classes.

jak0bw pushed a commit to jak0bw/MONAI that referenced this pull request Mar 28, 2023
SomeOf transform composer

### Description

Adds a new transform composer allowing the user to sample "some of" the
transforms from a given list of transforms. Allows sampling a fixed
number, or variable number of transforms within a configurable range.
Sampling is uniform and without replacement.

The Compose class does almost all of the heavy lifting. This class just
replaces the list of transforms each time with a newly sampled list of
transforms on each `__call__` invocation. The simple sampling entry
point will allow for easy extension later in case there's a need to add
support for other probability distributions.

This allows users to compose MONAI transforms in a non sequential way
without needing to write their own compositor, or wrapping compositors
from frameworks like Albumentations.

Maybe of interest to Issue Project-MONAI#5312

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] 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 --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Tuan Chien <[email protected]>
jak0bw pushed a commit to jak0bw/MONAI that referenced this pull request Mar 28, 2023
SomeOf transform composer

### Description

Adds a new transform composer allowing the user to sample "some of" the
transforms from a given list of transforms. Allows sampling a fixed
number, or variable number of transforms within a configurable range.
Sampling is uniform and without replacement.

The Compose class does almost all of the heavy lifting. This class just
replaces the list of transforms each time with a newly sampled list of
transforms on each `__call__` invocation. The simple sampling entry
point will allow for easy extension later in case there's a need to add
support for other probability distributions.

This allows users to compose MONAI transforms in a non sequential way
without needing to write their own compositor, or wrapping compositors
from frameworks like Albumentations.

Maybe of interest to Issue Project-MONAI#5312

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] 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 --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Tuan Chien <[email protected]>
@atbenmurray
Copy link
Contributor

I'd like to offer our belated thanks for your contribution @tuanchien. I am sure people will find this very useful!

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.

5 participants