Skip to content

1567 add RemoveRepeatedChannel#1569

Merged
wyli merged 17 commits intoProject-MONAI:masterfrom
leotam:1567-add-deletechanneld
Feb 25, 2021
Merged

1567 add RemoveRepeatedChannel#1569
wyli merged 17 commits intoProject-MONAI:masterfrom
leotam:1567-add-deletechanneld

Conversation

@leotam
Copy link
Copy Markdown
Contributor

@leotam leotam commented Feb 9, 2021

Signed-off-by: Leo Tam [email protected]

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.

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Feb 9, 2021

Leo Tam added 3 commits February 10, 2021 18:51
Signed-off-by: Leo Tam <[email protected]>
Signed-off-by: Leo Tam <[email protected]>
Signed-off-by: Leo Tam <[email protected]>
@ericspod ericspod requested a review from rijobro February 16, 2021 01:18
@rijobro
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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!

@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