Skip to content

3732 Update TTA module based on latest features#3733

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

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

Conversation

@Nic-Ma

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

Copy link
Copy Markdown
Contributor

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 and others added 10 commits February 1, 2021 19:15
@Nic-Ma

Nic-Ma commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

/black

@Nic-Ma

Nic-Ma commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

/build

@Nic-Ma

Nic-Ma commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

/build

Comment thread monai/data/test_time_augmentation.py
@Nic-Ma

Nic-Ma commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

/build

Comment thread monai/data/test_time_augmentation.py Outdated
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma

Nic-Ma commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

/build

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@Nic-Ma

Nic-Ma commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

/build

@rijobro

rijobro commented Jan 28, 2022

Copy link
Copy Markdown
Contributor

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

Nic-Ma commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

Hi @rijobro ,

Plan sounds good to me!

Thanks for your help here.

wyli
wyli previously approved these changes Jan 28, 2022

@wyli wyli left a comment

Copy link
Copy Markdown
Contributor

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
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro

rijobro commented Jan 31, 2022

Copy link
Copy Markdown
Contributor

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

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

@Nic-Ma

Nic-Ma commented Jan 31, 2022

Copy link
Copy Markdown
Contributor Author

@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
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@Nic-Ma

Nic-Ma commented Jan 31, 2022

Copy link
Copy Markdown
Contributor Author

/black

@Nic-Ma

Nic-Ma commented Jan 31, 2022

Copy link
Copy Markdown
Contributor Author

/build

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

wyli commented Jan 31, 2022

Copy link
Copy Markdown
Contributor

/build

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma

Nic-Ma commented Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

/build

@wyli wyli left a comment

Copy link
Copy Markdown
Contributor

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

Nic-Ma commented Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

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

wyli commented Feb 1, 2022

Copy link
Copy Markdown
Contributor

/build

@wyli

wyli commented Feb 1, 2022

Copy link
Copy Markdown
Contributor

/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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test time augmentation need to update with new features

3 participants