Skip to content

Conversation

@yiheng-wang-nv
Copy link
Contributor

@yiheng-wang-nv yiheng-wang-nv commented Aug 13, 2021

Signed-off-by: Yiheng Wang vennw@nvidia.com

Description

This PR fixes a fews issues mentioned in
Fixes #2759 , fixes #2760 and add some useful parameters for UpCat class.

Status

Ready

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 -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.

Signed-off-by: Yiheng Wang <vennw@nvidia.com>
@yiheng-wang-nv
Copy link
Contributor Author

yiheng-wang-nv commented Aug 13, 2021

Hi @ericspod @wyli @Nic-Ma , could you please help to look at this draft PR? The forward function of BasicUNetDecoder requires a sequence of tensors (represents all feature maps from the encoder) as input, and the sequence length is variable.

However, this implementation does not meet the requirements of torchscript. Do you have any suggestions to fix this issue? Thanks!

@wyli
Copy link
Contributor

wyli commented Aug 13, 2021

Hi @yiheng-wang-nv the basic unet is only a simple implementation with a focus on the code readability for the basic architecture, I feel this PR will add a bit too much complexity

I think you can create a separate standalone module called MultiscaleDecoder as a network block.

Signed-off-by: Yiheng Wang <vennw@nvidia.com>
Signed-off-by: Yiheng Wang <vennw@nvidia.com>
@yiheng-wang-nv
Copy link
Contributor Author

Hi @yiheng-wang-nv the basic unet is only a simple implementation with a focus on the code readability for the basic architecture, I feel this PR will add a bit too much complexity

I think you can create a separate standalone module called MultiscaleDecoder as a network block.

Thanks @wyli for the suggestion, I removed the decoder and I will submit a new PR for the implementation. Now this PR is only used to fix #2759 #2760 and add some upsample related parameters into UpCat (these are non-breaking changes).

@yiheng-wang-nv yiheng-wang-nv changed the title [WIP] add unetdecoder [WIP] Fix UpSample issue and enhance UpCat Aug 14, 2021
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

yiheng-wang-nv and others added 2 commits August 14, 2021 15:55
Signed-off-by: Yiheng Wang <vennw@nvidia.com>
@yiheng-wang-nv yiheng-wang-nv changed the title [WIP] Fix UpSample issue and enhance UpCat Fix UpSample issue and enhance UpCat Aug 14, 2021
@yiheng-wang-nv yiheng-wang-nv marked this pull request as ready for review August 14, 2021 08:02
@wyli wyli enabled auto-merge (squash) August 14, 2021 08:27
@wyli wyli merged commit 7ea09bf into Project-MONAI:dev Aug 14, 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.

UpSample has wrong docstrings and an issue with out_channels BasicUnet needs to be enhanced

2 participants