Skip to content
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 support for RECT_L #6727

Merged
merged 5 commits into from
Feb 17, 2023
Merged

Conversation

ftxj
Copy link
Contributor

@ftxj ftxj commented Feb 16, 2023

This PR add the TorchScript support for RECT_L model.

The fail reason and our solution for original code is very similar with PR #6721, except that this model using the torch.jit.export on embed and get_semantic_labels methods. And another fail reason is @torch.no_grad, which bring some error msg I cann't understand.

Adding TorchScript support will bring a lot of extra code and reduce the code readability. I will consider how to do better in another PR.

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #6727 (e230ce8) into master (c45f8db) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head e230ce8 differs from pull request most recent head 6358fe7. Consider uploading reports for the commit 6358fe7 to get more accurate results

@@           Coverage Diff           @@
##           master    #6727   +/-   ##
=======================================
  Coverage   89.20%   89.20%           
=======================================
  Files         425      425           
  Lines       23076    23084    +8     
=======================================
+ Hits        20585    20593    +8     
  Misses       2491     2491           
Impacted Files Coverage Δ
torch_geometric/nn/models/rect.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rusty1s rusty1s changed the title Add TorchScript support for RECT_L Add TorchScript support for RECT_L Feb 17, 2023
Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

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

Thanks. I like it. Your comment about code duplication makes a lot of sense to me, although I am not entirely sure how to do better here. Not sure if we can elegantly auto-generate these wrapper classes.

@rusty1s rusty1s enabled auto-merge (squash) February 17, 2023 14:58
@rusty1s rusty1s merged commit 392f501 into pyg-team:master Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants