Skip to content

Conversation

@Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Oct 19, 2020

Fixes #1117 .

Description

This PR added TorchScript tests for several networks, other networks contain some logic that can't support torch.jit.script and not easy to fix.

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

Nic-Ma and others added 4 commits October 15, 2020 08:04
[DLMED] fix torchscript issue in densenet (#1114)
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Oct 20, 2020

/black

@Nic-Ma Nic-Ma changed the title [WIP] 1117 add TorchScript compatible test 1117 Add TorchScript compatible test Oct 20, 2020
@Nic-Ma Nic-Ma marked this pull request as ready for review October 20, 2020 15:11
Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

It looks good other than the comment I made. I also stated in the related issue what needs to be done for the PSP class, that is the up8/16/32/64 members need to be always present so can be nn.Identity if not needed, and line 303 of ahnet.py can be interpolate_size = x.shape[2:]. I was able to get Torchscript to work with AHNet with these changes.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Oct 20, 2020

Hi @ericspod ,

Thanks very much for your review.
I have updated the PR according to your comments, now AHNet works, could you please help review it again?
There are some other networks that are not covered in this PR, because they still have different kinds of TorchScript errors that I didn't find quick fix, maybe we can try to investigate them one by one later?

Thanks.

Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

We can review the other networks later, I suspect some of the same issues are present in them as we saw here. Some of the reference networks meant to re-implement published work might not be compatible ever.

@ericspod ericspod merged commit d0b6677 into Project-MONAI:master Oct 21, 2020
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.

Add TorchScript compatible test for all the networks

2 participants