Skip to content

3732 Update TTA module based on latest features#3733

Merged
Nic-Ma merged 24 commits intoProject-MONAI:devfrom
Nic-Ma:optimize-tta
Feb 1, 2022
Merged

3732 Update TTA module based on latest features#3733
Nic-Ma merged 24 commits intoProject-MONAI:devfrom
Nic-Ma:optimize-tta

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented Jan 28, 2022

Fixes #3732 .

Description

This PR updated the TTA module with newer MONAI features based on the discussion with @dongyang0122 and @holgerroth .

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

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 28, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 28, 2022

/build

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 28, 2022

/build

wyli
wyli reviewed Jan 28, 2022
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 28, 2022

/build

rijobro
rijobro reviewed Jan 28, 2022
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 28, 2022

/build

@rijobro
add to optimize tta
4a87197 
Signed-off-by: Richard Brown <[email protected]>
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 28, 2022

/build

@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented Jan 28, 2022 •
edited
Loading

Hi @Nic-Ma, I've created a PR to add to your PR: Nic-Ma#362.

  1. Improve tests
  2. Reduce data type conversion.

The test currently fails, I will try to debug on Monday, but I feel it is important that it works before we merge. We seem to get different results for:

  1. numpy and torch inputs, and
  2. using the post_func instead of inferrer_fn=post_trans(model(x)).

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 28, 2022

Hi @rijobro ,

Plan sounds good to me!

Thanks for your help here.

wyli
wyli previously approved these changes Jan 28, 2022
View reviewed changes
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, it looks good to me.

@wyli wyli enabled auto-merge (squash) January 28, 2022 16:32
@wyli wyli disabled auto-merge January 28, 2022 16:34
@rijobro
Copy link
Copy Markdown
Contributor

rijobro commented Jan 31, 2022

@Nic-Ma my PR is ready: Nic-Ma#362.

Think you will need to merge it for the unit tests to work.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 31, 2022

@Nic-Ma my PR is ready: Nic-Ma#362.

Think you will need to merge it for the unit tests to work.

Hi @rijobro ,

Thanks for your enhancement, started CI tests in your PR and put some minor comments in it.

rijobro and others added 2 commits January 31, 2022 14:07
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 31, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 31, 2022

/build

@Nic-Ma Nic-Ma requested review from rijobro and wyli January 31, 2022 14:54
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Jan 31, 2022

/build

@Nic-Ma
[DLMED] fix flake8
7a89517 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 1, 2022

/build

wyli
wyli approved these changes Feb 1, 2022
View reviewed changes
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, it looks good to me. Minor issues (not introduced by this PR) are 1. Runtime error could be a warning to allow for randomness defined in the network such as dropout. 2 np.std has a ddof parameters but this is currently hardcoded cc @rijobro

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Feb 1, 2022

Hi @wyli ,

Thanks for your review.

  1. I changed RuntimeError to Warning and updated the unit tests in the latest commit.
  2. About the std arg, PyTorch also has another arg unbiased:
    https://pytorch.org/docs/stable/generated/torch.std.html
    I think maybe we don't need to add these additional args so far? If we got some requests, let's add it then?

What do you think?

Thanks.

@Nic-Ma Nic-Ma enabled auto-merge (squash) February 1, 2022 14:08
@wyli
Copy link
Copy Markdown
Contributor

wyli commented Feb 1, 2022

/build

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Feb 1, 2022

/build

@Nic-Ma Nic-Ma merged commit 397d511 into Project-MONAI:dev Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

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

@dongyang0122 dongyang0122 Awaiting requested review from dongyang0122

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

Test time augmentation need to update with new features

3 participants

@Nic-Ma @rijobro @wyli