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

Compile models and losses with TorchScript for training and inference #19

Merged
merged 3 commits into from
Dec 2, 2021

Conversation

RMeli
Copy link
Owner

@RMeli RMeli commented Dec 2, 2021

PR #11 did all the refactoring needed to compile the models with TorchScript but did not contain the last commit which implements the actual compilation. This PR pushes the missing commit.

@RMeli RMeli added the problem label Dec 2, 2021
@RMeli RMeli self-assigned this Dec 2, 2021
@RMeli
Copy link
Owner Author

RMeli commented Dec 2, 2021

The branch

if self.reduction == "mean":
    reduced_loss = torch.mean(loss)
elif self.reduction == "sum":
    reduced_loss = torch.sum(loss)

results in the following PyTorch error:

E       RuntimeError: 
E       
E       reduced_loss is not defined in the false branch:
E         File "/biggin/b195/lina3015/Documents/git/gnina-torch/gnina/losses.py", line 91
E                   loss = diff * diff
E           
E               if self.reduction == "mean":
E               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E                   reduced_loss = torch.mean(loss)
E                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E               elif self.reduction == "sum":
E               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E                   reduced_loss = torch.sum(loss)
E                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ <--- HERE
E           
E               return self.scale * reduced_loss
E       and was used here:
E         File "/biggin/b195/lina3015/Documents/git/gnina-torch/gnina/losses.py", line 96
E                   reduced_loss = torch.sum(loss)
E           
E               return self.scale * reduced_loss
E                                   ~~~~~~~~~~~~ <--- HERE

The problem is that the if/elif block does not cover all execution paths leading to self.scale * reduced_loss and for the missing else clause the reduced_loss variable is undefined.

Using if/else is safe because of the assertion

assert reduction in ["mean", "sum"]

and solves the problem since reduced_loss is defined for all execution branches.

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #19 (d34c1e6) into main (39985f3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@RMeli RMeli merged commit f8aecb6 into main Dec 2, 2021
@RMeli RMeli deleted the jit branch December 2, 2021 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant