-
Notifications
You must be signed in to change notification settings - Fork 15
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 options for eval and gradient required #78 - rebase #103
Conversation
Hi @ElliottKasoar after the work @jwallwork23 did there were some conflicts with your PR in #78. I have done my best to rebase your work, but please could you take a look and see if everything seems in order to you? @TomMelt You originally reviewed this PR and approved, but if you could take a quick glance and re-review it would be appreciated - since @jwallwork23 restructured the order of functions in the files and added additional arguments in the same place as @ElliottKasoar some of the merge conflicts got a little hairy so I may have missed the odd thing! |
Before we merge we need to:
|
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.
Nice, seems like useful functionality.
Just a couple of comments that we need to address either by following my recommendations or by updating the n_c_and_cpp
example to use the new API.
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! Looks great other than a couple of places where I think the code may have got lost in the rebase (unless intentional?)
3cd9aca
to
75b7aa0
Compare
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.
One minor point on formatting but otherwise looks good to me!
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.
Looks good, thanks @jatkinson1000!
…s_grad arguments to match Fortran.
…tKasoar Co-authored-by: ElliottKasoar <[email protected]>
1a8a79b
to
2206e39
Compare
Added a note to the FAQ about eval and no_grad settings. I will also move some of @ElliottKasoar's points in his original comment to separate issues for future consideration. Squashing and merging shortly. |
This is an updated version of #78 rebased onto main after the GPU changes by @jwallwork23
@ElliottKasoar's comment on the original PR:
Resolves #73
Adds flags in all(?) functions that operate on tensors (tensor creation, model loading, forward) to optionally disable autograd, which should improve performance for inference.
Also adds a similar flag to set evaluation mode for the loaded model.
Evaluation mode
NoGradMode
with torch.no_grad():
).InferenceMode
Model freezing
(For more general explanation of autograd/evaluation mode, see autograd mechanics).
Note: I've also removed the old, commented out
torch_from_blob
function.