Skip to content

1117 Add TorchScript compatible test#1121

Merged
ericspod merged 8 commits intoProject-MONAI:masterfrom
Nic-Ma:1117-add-torchscript-compatible-test
Oct 21, 2020
Merged

1117 Add TorchScript compatible test#1121
ericspod merged 8 commits intoProject-MONAI:masterfrom
Nic-Ma:1117-add-torchscript-compatible-test

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented Oct 19, 2020 •
edited
Loading

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 Nic-Ma mentioned this pull request Oct 19, 2020
Closed
@Nic-Ma
Copy link
Copy Markdown
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
ericspod
ericspod reviewed Oct 20, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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.

ericspod
ericspod approved these changes Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@ericspod ericspod ericspod approved these changes

@wyli wyli Awaiting requested review from wyli

@atbenmurray atbenmurray Awaiting requested review from atbenmurray

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add TorchScript compatible test for all the networks

2 participants

@Nic-Ma @ericspod