Skip to content

Add parameter unpacking for transforms#2437

Merged
wyli merged 4 commits intoProject-MONAI:devfrom
Spenhouet:feature/2426-unpack-transform-params
Jun 24, 2021
Merged

Add parameter unpacking for transforms#2437
wyli merged 4 commits intoProject-MONAI:devfrom
Spenhouet:feature/2426-unpack-transform-params

Conversation

@Spenhouet
Copy link
Copy Markdown
Contributor

@Spenhouet Spenhouet commented Jun 24, 2021 •
edited
Loading

Signed-off-by: Sebastian Penhouet [email protected]

Fixes #2426

Description

Transforms can return any number of values. This pull request enables that these return values are passed into the next transform in a Compose as parameters (unpacked arguments).
This is especially useful in the case where the input and output of transforms always matches like in the case of always passing along the image array and the meta data dict.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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.

Add parameter unpacking for transforms
1fccbb9 
Signed-off-by: Sebastian Penhouet <[email protected]>
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Jun 24, 2021

/integration-test

@Spenhouet
Copy link
Copy Markdown
Contributor Author

Spenhouet commented Jun 24, 2021

mypy issues:

monai/engines/utils.py:144:16: error: Incompatible types in assignment (expression has type "Union[List[Dict[Any, Any]], Dict[Any, Any]]", variable has type "Dict[Any, Any]")  [assignment]
monai/data/image_dataset.py:127:38: error: "None" object is not iterable  [misc]
monai/data/dataset.py:1062:20: error: Incompatible types in assignment (expression has type "Union[List[Dict[str, Any]], Dict[str, Any]]", variable has type "Dict[str, Any]")  [assignment]

Strange that I did not get them on my local development. I will look into it.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Jun 24, 2021

Hi @Spenhouet ,

Thanks for your quick PR, I think mypy usually has big difference between versions, please update your local mypy to 0.902 and try again.

Thanks.

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Jun 24, 2021

/integration-test

@Spenhouet
Copy link
Copy Markdown
Contributor Author

Spenhouet commented Jun 24, 2021

@wyli @Nic-Ma I fixed the mypy complains (and also the usage in ImageDataset).

wyli
wyli approved these changes Jun 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks!

@Spenhouet
Copy link
Copy Markdown
Contributor Author

Spenhouet commented Jun 24, 2021

Btw. I did set the unpack flag to False by default to not break any current behavior.

@wyli wyli enabled auto-merge (squash) June 24, 2021 21:28
@wyli wyli merged commit 38403bb into Project-MONAI:dev Jun 24, 2021
Nic-Ma
Nic-Ma reviewed Jun 24, 2021
View reviewed changes
monai/data/dataset.py
transformed_data = apply_transform(self.transform, data)

if not isinstance(transformed_data, dict):
raise AssertionError("With a dict supplied to apply_transform a single dict return is expected.")
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma Jun 24, 2021

Choose a reason for hiding this comment

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

Hi @Spenhouet ,

Thanks very much for your quick PR!
I think this assert is not correct here, some MONAI sampling transforms return a list of dictionaries instead of a single dict.
I will submit a quick PR to fix it soon, CC @wyli @ericspod @rijobro .
Others of this PR look good to me.

Thanks.

@Nic-Ma Nic-Ma mentioned this pull request Jun 25, 2021
Merged
7 tasks
@wyli wyli mentioned this pull request Jun 25, 2021
Merged
1 task
@Nic-Ma Nic-Ma mentioned this pull request Jan 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nic-Ma Nic-Ma Nic-Ma left review comments

@wyli wyli wyli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multiple transform return values are not properly passed as parameters for next transform

3 participants

@Spenhouet @wyli @Nic-Ma