RandomizableTransformType, LazyTransformType, and MultiSampleTransformType #5410
Conversation
…mpleTransform. RandomizableTransform now inherits from IRandomizableTransform
monai/transforms/transform.py
Outdated
| raise NotImplementedError() | ||
|
|
||
|
|
||
| class IRandomizableTransform: |
There was a problem hiding this comment.
I understand that we need IMultiSampleTransform, but I'm not sure about IRandomizableTransform or ILazyTransform, these are not required concepts at the moment according to the prototypes... I can redirect my PR (#5407) to your fork if that addresses any potential merging conflicts
There was a problem hiding this comment.
If you look at the dictionary transform implementations on #4922, they make use of IRandomizableTransform.
The point of IRandomizableTransform is twofold:
- It can be the way that a transform is detected as randomizable or not, regardless of whether it is implemented in MONAI or extended from another library
- Randomizable dictionary transforms inherit from RandomizableTransform but then create a randomizable array transform to do the heavy lifting. However, they only delegate most of the randomization to the array class, whereas they could delegate all of it. IRandomizableTransform would allow that to be fixed as the dictionary transform can just say "hey, I'm randomizable" and then delegate all of its work to the array transform rather than having an implementation that is only partially used
The addition of this interface is partially motivated by the way I've implemented RandRotated in the big PR #4922 .
I might have gone too far in creating a separate Randomizer class that can be reused across the dictionary and array implementations, but I think it results in pretty clean code.
There was a problem hiding this comment.
I see, that looks like another refactoring effort introducing new concepts of Randomizer and IRandomizableTransform... how about we limit the scope for lazy resampling feature, and all the other nice-to-have features stay on a feature branch for now?
really hope we can ship the lazy resampling soon. The other refactorings add much risks but at the moment they are not critical from a user experience's perspective...
|
Prefixing interfaces with |
It's neither pythonic nor un-pythonic, as there isn't really anything either way on how to name interface style classes. That said, I get your point. The problem is that we have both IRandomizableTransform (the signature) and RandomizableTransform (some implementation). I would definitely suggest keeping them separate if at all possible |
Signed-off-by: Wenqi Li <[email protected]> Fixes #5414 ### 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). - [x] 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: Wenqi Li <[email protected]>
|
atbenmurray
changed the title
…mpleTransform. RandomizableTransform now inherits from IRandomizableTransform Adding license text Adding entries to docs; removing obsolete import from irandomizabletransform tests Adding interfaces to transforms/__init__.py Renaming interface types after feedback Updating test class / method names to reflect name change to RandomizableTransformType Accepting autoformatting fixes Updating docs with revised type names Signed-off-by: Ben Murray <[email protected]>
…r_interfaces Signed-off-by: Ben Murray <[email protected]>
…mpleTransform. RandomizableTransform now inherits from IRandomizableTransform Adding license text Adding entries to docs; removing obsolete import from irandomizabletransform tests Adding interfaces to transforms/__init__.py Renaming interface types after feedback Updating test class / method names to reflect name change to RandomizableTransformType Accepting autoformatting fixes Updating docs with revised type names Signed-off-by: Ben Murray <[email protected]> Implementation of IRandomizableTransform, ILazyTransform and IMultiSampleTransform. RandomizableTransform now inherits from IRandomizableTransform Adding license text Adding entries to docs; removing obsolete import from irandomizabletransform tests Renaming interface types after feedback Accepting autoformatting fixes Updating docs with revised type names Signed-off-by: Ben Murray <[email protected]>
…r_interfaces Signed-off-by: Ben Murray <[email protected]>
|
Not going ahead with this one; having issues with signing. Started a duplicate PR #5418 |
…mpleTransform. RandomizableTransform now inherits from IRandomizableTransform
Fixes # .
Description
A few sentences describing the changes proposed in this pull request.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.