-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add TorchScript #116
Add TorchScript #116
Conversation
@kmkurn I rebased this onto the master branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for continuing the torchscript PR! Overall it looks good, but I left some requests and questions below.
One major request is about the structure of the tests. Do you mind changing it so that each assert is in its own test function/method? Similar to the tests for the non-scripted version. It will make pytest's report a lot clearer when a test fails. Feel free to create another test module under tests
if necessary.
I've split up the tests into smaller pieces. The commit messages are a bit messy, I think I'll squash them into one if it is ready to get merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes and clarifications. It looks really good now. I just have a question and minor requests below before I can merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! Just a couple of minor requests from me, and the PR should be ready to merge.
I think this PR looks good to merge. If you still want to squash the commits into one, please do so now, and then let me know. |
@kmkurn Thanks! The commits are squashed into one. |
Thank you for your contribution! The PR is now merged :-) |
This is my attempt to address the reviews of #113 .