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 rand rot90 transforms and dictionary-based counterparts #106

Merged
merged 14 commits into from
Feb 28, 2020

Conversation

wyli
Copy link
Contributor

@wyli wyli commented Feb 20, 2020

relevant issues #72 #58
fixes #59 fixes #63

Description

adds

  • rotate90 transform
  • randomised rotate90 transform

in vanilla and dictionary-based formats.

organises:

  • vanilla transforms (callables that deal with image array) in monai/transforms/transforms.py
  • dictionary transforms (callables that deal with data dict) in monai/transforms/composables.py
  • randomizable interface for random augmentation transforms

could you have a look?
@atbenmurray @ericspod @Nic-Ma @yanchengnv
it could be a starting point for further discussions

Status

READY

Types of changes

  • Breaking change (fix or new feature that would cause existing functionality to change)
  • New tests added to cover the changes
  • Docstrings/Documentation updated

@wyli wyli added this to In progress in Public alpha via automation Feb 20, 2020
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Added comments inline.
Thanks.

Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Added comments inline.
Thanks.

monai/transforms/rand_rotation.py Outdated Show resolved Hide resolved
@wyli wyli force-pushed the 63-transform-wrapper branch 2 times, most recently from 32b084a to 5502dde Compare February 24, 2020 21:13
@wyli
Copy link
Contributor Author

wyli commented Feb 24, 2020

before a proper adaptor is in place I reckon that we go for dictionary-based wrappers around each vanilla transform. this PR's Rotate90d and RandRotate90 are dictionary-based. users could also choose to use the vanilla directly or build customised adaptors around them. @atbenmurray @ericspod

Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

Very minor changes but otherwise

monai/transforms/rand_rotation.py Outdated Show resolved Hide resolved
monai/transforms/rand_rotation.py Outdated Show resolved Hide resolved
monai/transforms/rand_rotation.py Outdated Show resolved Hide resolved
monai/transforms/rand_rotation.py Outdated Show resolved Hide resolved
@wyli
Copy link
Contributor Author

wyli commented Feb 28, 2020

thanks for the online/offline discussions, @ericspod @atbenmurray @Nic-Ma @yanchengnv , I think we are making good progress here.

notebly, this PR have organised

  • vanilla transforms (callables that deal with image array) in monai/transforms/transforms.py
  • dictionary transforms (callables that deal with data dict) in monai/transforms/composables.py
  • randomizable interface for random augmentation transforms

I'll update the unit tests and merge (still open to any comments/suggestions if any)

@wyli wyli changed the title add rand rot90 transforms add rand rot90 transforms and dictionary-based counterparts Feb 28, 2020
@wyli wyli merged commit 86e9e54 into master Feb 28, 2020
Public alpha automation moved this from In progress to Done Feb 28, 2020
@wyli wyli deleted the 63-transform-wrapper branch April 6, 2020 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Public alpha
  
Done
Development

Successfully merging this pull request may close these issues.

Finalize the design pattern for Transform. Implement sequential Compose functionality
3 participants