Skip to content

adds an acti-norm-dropout block #1120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
wyli merged 4 commits into from
Oct 21, 2020
Merged

adds an acti-norm-dropout block #1120

wyli merged 4 commits into from
Oct 21, 2020

Conversation

@wyli
Copy link
Contributor

@wyli wyli commented Oct 19, 2020

Description

  • adds a sequential module of acti-norm-dropout (with flexible ordering)
  • fixes norm.group factory to be consistent with batch/instance norm

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 --codeformat --coverage.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Oct 20, 2020

Hi @ericspod ,

Could you please help review this PR? I am not very familiar with the background.
Thanks.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Oct 20, 2020

Hi @yiheng-wang-nv ,

Could you please also help review this PR?
Thanks.

@ericspod
Copy link
Member

ericspod commented Oct 20, 2020 •
edited
Loading

We're currently discussing how to ensure Torchscript compatibility where we can, has this new module been tested for that? I know the current version of Convolution does work. Other than that this is good to go.

yiheng-wang-nv
yiheng-wang-nv approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@yiheng-wang-nv yiheng-wang-nv left a comment

Choose a reason for hiding this comment

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

Thanks, this new block is very helpful.

@wyli wyli force-pushed the adds-acti-norm-dropout-block branch 2 times, most recently from 3c71101 to 8e7d659 Compare October 21, 2020 09:58
wyli added 3 commits October 21, 2020 11:02
@wyli
adds an acti-norm-dropout block
6e0f7be 
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
fixes unit tests
0183355 
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
update docstrings
83ac009 
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the adds-acti-norm-dropout-block branch from 8e7d659 to 83ac009 Compare October 21, 2020 10:11
@wyli
1D test cases
bb6f3af 
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the adds-acti-norm-dropout-block branch from bba0e05 to bb6f3af Compare October 21, 2020 11:10
@wyli
Copy link
Contributor Author

wyli commented Oct 21, 2020

We're currently discussing how to ensure Torchscript compatibility where we can, has this new module been tested for that? I know the current version of Convolution does work. Other than that this is good to go.

thanks, I've checked this change doesn't break the current torchscript tests

@wyli wyli merged commit 53d14cb into Project-MONAI:master Oct 21, 2020
@yiheng-wang-nv yiheng-wang-nv mentioned this pull request Mar 4, 2021
Closed
@wyli wyli deleted the adds-acti-norm-dropout-block branch April 12, 2021 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yiheng-wang-nv yiheng-wang-nv yiheng-wang-nv approved these changes

@ericspod ericspod Awaiting requested review from ericspod

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wyli @Nic-Ma @ericspod @yiheng-wang-nv