OneOf Transform

[lyndonboone]

Description

Added OneOf transform (subclass of Compose) similar to same name transform in TorchIO

Intended to close #1847

Status

ready (inverse impl. for the dict-based inputs)

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.

[madil90]

/build

[madil90]

/build

[madil90]

/build

[madil90]

/build

[rijobro]

Thanks, the update looks good. Some tests are required though before the PR can be merged.

I think it would also be really useful to have a go at the inverse method. It should be fairly straightforward. When the method gets called, you just need to store the index of the randomly selected transform. In the inverse direction, you use self.transforms[index].inverse(). Not all transforms have inverse methods, of course, so there would need to be some isinstance(self.transform[index], InvertibleTransform) logic there.

[lyndonboone]

Thanks! I'll try to write some unit tests for it. Would it be best to reference the ones in test_compose.py?

I can definitely try to have a go at the inverse method as well. Will give it a try and update the PR when finished

[rijobro]

test_compose might not be the best to use as reference, perhaps just a normal transform would be better. Something like test_rand_flip, for example. You'll want to supply a few transforms and then ensure that if called enough times, they get called at a reasonable percentage (e.g., if you have weightings of 1, 1, 2, you would expect to have 0.25, 0.25, 0.5 splits. You can create blank transforms so that it is very quick, and you can easily run 100 times. That's just one example of a test, I'm sure there are many other sanity checks.

[Nic-Ma]

Hi @lyndonboone @rijobro ,

I think the flatten() API of this OneOf compose is not correct, if using base class's flatten(), it will convert OneOf to Compose, right?

Thanks.

[rijobro]

Good point. Do you think we should just raise a NotImplementedError? The alternative is recursively working through list and if we have nested OneOf transforms, we would need to multiply the probabilities of each depth. Sounds like more work than it's worth.

e.g.,

OneOf:
    w=0.5, Flip
    w=0.5, OneOf:
        w=0.25, Rotate
        w=0.75, Resize

would become

OneOf:
    w=0.5, Flip
    w=0.125, Rotate
    w=0.375, Resize

[Nic-Ma]

Hi @rijobro ,

Thanks for sharing the idea.
I re-think your idea, seems NotImplementedError is better choice.

Thanks.

[Nic-Ma]

Hi @lyndonboone ,

May I know your plan for this PR? Do you have some bandwidth to complete it in a short time?

Thanks in advance.

[lyndonboone]

Hi @Nic-Ma ,

My apologies, I got a little tied up with some other things over the past few weeks. I should be able to get back to this early next week if that works.

[Nic-Ma]

Hi @lyndonboone ,

Cool, plan sounds good!
Thanks for your contribution.

[lyndonboone]

Hi @Nic-Ma , @rijobro ,

Just catching up with the discussion. I was wondering what to do with the flatten transform. Is it decided that we just raise NotImplementedError? If so, what should we do with the __len__ method it inherits since it uses flatten?

Also, do you think we should use **kwargs in the constructor for any arguments it shares with Compose?

[rijobro]

Hi @lyndonboone, I just added the flatten method, so we get the len method for free. I also added the start of the unit tests, feel free to flesh those out.

P.S. I wanted to create a PR to your branch but accidentally pushed straight to it. Sorry about that! Feel free to review the modifications and make any changes you might want.

[rijobro]

also i don't think we should do the **kwargs. We have quite a lot of inheritance in our transforms and it would get quite confusing to use kwargs for all of those, since the user would have to check through the documentation for each of the inherited classes.

[lyndonboone]

Oh awesome, thanks! No worries at all.

I'll try to have a go at the unit tests and inverse method

[lyndonboone]

Hi @rijobro ,

Just added a few more unit tests and an inverse method. Let me know if all looks good and if anything can be improved.

[neuronflow]

very cool to see my feature request become reality, thanks for your efforts! 🚀

[Nic-Ma]

Seems we use warning instead of `exception at other places:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/inverse_batch_transform.py#L47