Skip to content

Conversation

@Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Sep 10, 2021

Fixes #2920 .

Description

This PR enhanced the padding modes and additional args for Tensor data.

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.

@Nic-Ma Nic-Ma requested review from rijobro and wyli September 10, 2021 02:25
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 10, 2021

Hi @wyli and @rijobro ,

I enhanced the padding mode and addtional args in the Pad transform in this draft PR, could you please help review the proposal first?
If you agree with the proposal, I will try to update all the related places in the codebase in this PR.

Thanks in advance.

@rijobro
Copy link
Contributor

rijobro commented Sep 10, 2021

It would certainly be great if we could use torch.pad for torch input and numpy.pad for numpy input. I think the reason I made these caveats was because the torch.pad is more limited compared to the numpy version. I suspect this will be reflected by the unit tests, but would be great if you could get it working!

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 10, 2021

Hi @rijobro ,

Thanks for your support, I think it's user's responsibility to choose the correct mode for the input data type(numpy or Tensor), we can explicitly show the supported modes of numpy and Tensor in the doc-string. If the passing mode is not compatible, it will raise an error naturally.

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 10, 2021

Hi @wyli ,

Do you have any other concerns?
If no, I will try to complete this PR for all the related components following this direction: numpy input -> np.pad, tensor input -> torch.pad, rename np_kwargs -> kwargs.

Thanks.

Thanks.

@wyli
Copy link
Contributor

wyli commented Sep 10, 2021

do we need a suitable default for both numpy and torch inputs?

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 10, 2021

do we need a suitable default for both numpy and torch inputs?

Maybe let's change the default mode to constant? @wyli @rijobro What do you guys think?

Thanks.

@wyli
Copy link
Contributor

wyli commented Sep 10, 2021

constant 0 may 'break' the original intensity distribution and the results of mean/max/min would be very different from the original ones...I think 'edge'/'replicate' is a safer choice

@Nic-Ma Nic-Ma changed the title [WIP] 2920 Enhance padding mode for Tensor data 2920 Enhance padding mode for Tensor data Sep 11, 2021
@Nic-Ma Nic-Ma marked this pull request as ready for review September 11, 2021 16:11
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 11, 2021

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 11, 2021

Hi @wyli , @rijobro ,

I have updated the PR based on the previous discussion, mapping numpy and torch modes to make it compatible.
Could you please help review it?
I didn't update the transforms that don't support Tensor data yet.

Thanks in advance.

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 11, 2021

/black

Signed-off-by: Nic Ma <nma@nvidia.com>
@wyli
Copy link
Contributor

wyli commented Sep 11, 2021

/build

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, please see some minor comments inline

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma force-pushed the 2920-enhance-tensor-pad branch from caff2ac to 153a7ef Compare September 12, 2021 01:48
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Sep 12, 2021

/black

@Nic-Ma Nic-Ma enabled auto-merge (squash) September 12, 2021 01:49
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma merged commit c50235c into Project-MONAI:dev Sep 12, 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.

RandZoom change the input type

3 participants