-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Randomizable constructor #1639
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
Randomizable constructor #1639
Conversation
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
…ctor Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
…ctor Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
|
@wyli any chance you could have a look at this? Hoping to get it merged so I can move onto the next set of PRs. |
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
thanks I'll need more time to check the details as there's no unit test to cover the changes...
|
Sure, my metric was just that none of the previous unit tests were broken. Slightly worrying that the code format wasn't picking up any warnings with this one... https://github.com/Project-MONAI/MONAI/blob/master/monai/apps/datasets.py#L136 EDIT: oh, it's because it doesn't call the base class. |
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
e9b7b81 to
9edb551
Compare
|
@wyli updated to add a |
|
/integration-test |
wyli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, it looks good to me
| return self | ||
|
|
||
| @abstractmethod | ||
| def randomize(self, data: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we support None here and in the RandomizableTransform? (if it doesn't break anything...)
| def randomize(self, data: Any) -> None: | |
| def randomize(self, data: Optional[Any] = None) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shouldn't have enabled automatic merging!
I would quite like the default of data for Randomizable and RandomizableTransform to default to None. Currently I have to use a lot of super().randomize(None), but I'd rather have super().randomize().
However, this is incompatible with certain child classes. E.g., RandFlip takes np.ndarray as argument, which is incompatible with None.
The alternative is to switch all child classes from e.g., np.ndarray to Optional[np.ndarray], but that doesn't make sense either, because the np.ndarray in this case isn't optional.
Another alternative is just to ignore the mypy errors for those classes.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! it's just a very minor issue, I think the current one is also good, I forgot the mypy errors...
Adds randomizable constructor.
Description
This allows the parent class to store
proband_do_transform.I'll need this for the inverse transformation PR.
#1515
Status
Ready