Skip to content

optional strict_check for EnsureChannelFirst(d)#2309

Merged
Nic-Ma merged 2 commits intoProject-MONAI:devfrom
wyli:fixes-ensure-channel
Jun 7, 2021
Merged

optional strict_check for EnsureChannelFirst(d)#2309
Nic-Ma merged 2 commits intoProject-MONAI:devfrom
wyli:fixes-ensure-channel

Conversation

@wyli
Copy link
Copy Markdown
Contributor

@wyli wyli commented Jun 4, 2021

Signed-off-by: Wenqi Li [email protected]

Description

adds a boolean flag strict_check to the EnsureChannelFirst

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.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@wyli
optional strict check
dbf4533 
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli requested review from Nic-Ma, ericspod and rijobro June 4, 2021 11:28
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Jun 4, 2021

Hi @wyli ,

Thanks for your enhancement for EnsureChannelFirstd.
But I think if no meta data provided or no original_channel_dim found, EnsureChannelFirstd can't work as expected at all, so we should raise the exception instead of just a warning.
Maybe you have some use cases for it, could you please help share it for further discussion?

Thanks.

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jun 4, 2021

this is useful when customising preprocessors and composes, e.g. I was experimenting with this example, using ensure channel and changing the flags.

MONAI/monai/data/dataset.py

Lines 931 to 943 in c1e06f5

class TestCompose(Compose):
def __call__(self, input_):
img, metadata = self.transforms[0](input_)
img = self.transforms[1](img)
img, _, _ = self.transforms[2](img, metadata["affine"])
return self.transforms[3](img), metadata
img_transform = TestCompose(
[
LoadImage(image_only=False),
AddChannel(),
Spacing(pixdim=(1.5, 1.5, 3.0)),
RandAdjustContrast()
]

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Jun 4, 2021

Thanks for your explanation.
I feel when users use EnsureChannelFirstd transform, they expect it to automatically guarantee a channel-first data, if not, raise exception instead of "can't work and return the original data" with a warning.
For experiment purpose, maybe you can easily comment this transform when you change the code to test something?
Anyway, it's not a breaking change, if @ericspod and @rijobro agree with it, I am OK to approve it.

Thanks.

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jun 4, 2021 •
edited
Loading

if that's the original intention then the previous function signature is also wrong meta_dict: Optional[Dict] = None, it shouldn't accept None by default

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Jun 4, 2021

Hi @wyli ,

Thanks for your finding.
I agree to change the signature from meta_dict: Optional[Dict] = None to meta_dict: Dict as the original check in the function already strongly require it to be a dict. Maybe I made the mistake when developing this transform.
Could you please help change this PR to update the signature of the function?

Thanks.

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Jun 4, 2021

Hi @wyli ,

Thanks for your finding.
I agree to change the signature from meta_dict: Optional[Dict] = None to meta_dict: Dict as the original check in the function already strongly require it to be a dict. Maybe I made the mistake when developing this transform.
Could you please help change this PR to update the signature of the function?

Thanks.

this PR addressed a previous error, added more flexibility, kept the previous behaviour. I really don't see an issue here.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Jun 4, 2021

But I think maybe the previous issue is that: the API signature is not defined clearly?
@ericspod @rijobro what do you think?

Thanks.

Nic-Ma
Nic-Ma approved these changes Jun 7, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Since @ericspod @rijobro don't have any comments and this PR is a non-breaking enhancement, I am OK to merge it.

Thanks.

@Nic-Ma Nic-Ma enabled auto-merge (squash) June 7, 2021 03:34
@Nic-Ma Nic-Ma merged commit 509c2c1 into Project-MONAI:dev Jun 7, 2021
@wyli wyli deleted the fixes-ensure-channel branch June 7, 2021 10:10
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 approved these changes

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

@rijobro rijobro Awaiting requested review from rijobro

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wyli @Nic-Ma