Skip to content

Conversation

@leotam
Copy link
Contributor

@leotam leotam commented Feb 9, 2021

Signed-off-by: Leo Tam leot@nvidia.com

Fixes #1567 .

Description

Add DeleteChannel inverse operation to RepeatChannel via slicing. I tried squashing the commits which hilarious duplicated them instead. Anyways if it passes review, can fight git more.

Status

Work in progress

Types of changes

  • 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).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh --codeformat --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Leo Tam added 4 commits February 9, 2021 00:17
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
@wyli
Copy link
Contributor

wyli commented Feb 9, 2021

thanks, adding the new class to this file will address some of the ci/cd errors https://github.com/Project-MONAI/MONAI/blob/master/monai/transforms/__init__.py#L258

Leo Tam and others added 4 commits February 9, 2021 17:23
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
Leo Tam added 3 commits February 10, 2021 18:51
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
@ericspod ericspod requested a review from rijobro February 16, 2021 01:18
@rijobro
Copy link
Contributor

rijobro commented Feb 16, 2021

I find the name slightly confusing. I would expect DeleteChannel to have argument idx: Union[Sequence[int], int], deleting the channels at the specified index/indices. Since this is the inverse of RepeatChannel I think the name should match that more closely, e.g., RemoveRepeatedChannel.

Alternatively, an inverse method could be added to #1514. I've only concentrated on spatial transformations so far, but there's no reason it has to be limited to these use cases (the base class is InvertibleTransform).

@leotam
Copy link
Contributor Author

leotam commented Feb 16, 2021

That's probably clearer to fit the use case. As for adding the inverse method to your spatial transforms PR, the hesitation would be it bloats the scope of that PR as it's a nontrivial expansion of scope, and makes it harder to test.

Discussing tests, what's this windows-latest CPU test failing? There's no clear error message. @wyli any helpful comments for these two items?

@rijobro
Copy link
Contributor

rijobro commented Feb 16, 2021

I think the relevant part of the Windows error is here. It's trying to use pretrained FCN, but failing to download the pretrained network.

@wyli
Copy link
Contributor

wyli commented Feb 18, 2021

agree that this transforms is removing channels according to the 'repeated' pattern, I think RemoveRepeatedChannel is more appropriate...
we could have a separate PR to inverse the repeat channel transform, we need #1514 to provide some generic interfaces

@leotam leotam changed the title 1567 add deletechanneld 1567 add RemoveRepeatedChannel Feb 25, 2021
Leo Tam added 3 commits February 25, 2021 18:52
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
Signed-off-by: Leo Tam <leot@nvidia.com>
Copy link
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!

@wyli wyli enabled auto-merge (squash) February 25, 2021 19:21
@wyli wyli merged commit f83e300 into Project-MONAI:master Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add DeleteChannelD inverse equivalent of RepeatChannelD

3 participants