-
Notifications
You must be signed in to change notification settings - Fork 29
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
Energy forces #278
Merged
Merged
Energy forces #278
Changes from 7 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
7b56afd
Energy and Force Prediction changes (loss function in base, and optio…
RylieWeaver a277ee5
comments and renamings
RylieWeaver fd248b5
Black formatting and mak computational graph fixes
RylieWeaver c3eaedc
fix loss weighting
RylieWeaver 5136b89
black formatting
RylieWeaver 65f6f1d
black formatting
RylieWeaver 3395484
black formatting
RylieWeaver c5d0c7b
Fix DIMEStack testing issues, and compute_grad_energy default in conf…
RylieWeaver fc5a554
LJ example added first draft
RylieWeaver 0bce3d5
formatting
RylieWeaver 271e36a
formatting
RylieWeaver 9de9677
formatting
RylieWeaver a904def
formatting
RylieWeaver 12c042e
formatting
RylieWeaver 0f3fe54
formatting
RylieWeaver 4df57c7
take out images and adjust unecessary import
RylieWeaver 4779309
revert to SiLU in DimeNet
RylieWeaver 456ebf7
don't create plots by default
RylieWeaver 20f2e41
file cleanup and dataset test
RylieWeaver 2da742e
unnecessary import
RylieWeaver 70206dc
file cleanup
RylieWeaver 7032acd
add some tests back in
RylieWeaver f98fb3e
Restore all tests
RylieWeaver d66de16
formatting
RylieWeaver 206b52f
check dataset things
RylieWeaver 9980834
formatting
RylieWeaver fa10c20
Revise paths to be more succinct and Use radius from config
RylieWeaver 42551b8
file restructuring and using hydra radius graph function
RylieWeaver ee20cf7
formatting
RylieWeaver 1fa7766
renaming
RylieWeaver e8a4a4d
remove qm9 test
RylieWeaver 5d358a3
remove qm9 test
RylieWeaver 06dad51
smaller number samples for test
RylieWeaver 393976c
Update examples/LennardJones/LJ_inference_plots.py
allaffa 85f44ab
use info function
RylieWeaver 81992af
Unnecessary __init__ file
RylieWeaver bc93b9b
Unecessary json args
RylieWeaver File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why is
grad_outputs
used here? This would lead to sum of the gradients, right?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.
I don't think so.
Here's my understanding of it:
The
grad_outputs
argument essentially serves as a weight to multiply the gradients. This is necessary sincegraph_energy_pred
is a higher dimension than just a scalar. The torch.ones_like() here will assign a weight of 1 to each gradient, which I believe is what we want. Otherwise, there would be a force prediction which is c*grad_E where c is a constant not equal to 1, which is nonphysical.Specifically to your question:
No, I don't believe that it results in a sum. The output shape of forces_pred will be in the same shape as data.pos. The grad_outputs specifically is for weighting the gradients before a sum, but that extra step of summing is outside of autograd calculation I believe.
Does this answer your question?
@pzhanggit
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. So just to verify if my understanding is correct:
grad_outputs
is needed because we're calculating gradients of a batch of samples here. And sincegrad_outputs should be a sequence of length matching output containing the “vector” in vector-Jacobian product
, it provides a way to aggregate/sum the gradients across all the samples in the batch. This is equivalent to calculate the gradients iteratively for each sample, since the cross-gradients between samples would be zero.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.
@pzhanggit
Yep, that's right.
I think so. Yes,
grad_outputs
should be a sequence, and it should be the same shape as whatever you're predicting. It then multiplies the matrix of gradients (row-wise, not normal matrix multiplication), which I think is thevector-Jacobian product
you're referring to.I think yes. It's scaling those gradients, which would be relevant in an aggregation/sum. Although, it does not do that aggregation/sum.
Yep.
Are there any parts still unclear?
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.
Scaling those gradients
?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.
@pzhanggit
The scaling is done with
torch.ones_like(graph_energy_pred)
In our case, we use vectors of ones because we do NOT want to scale. However, you could apply any multiplying factor (or even provide a customized vector with different values for each entry).
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.
@pzhanggit @allaffa ^ on what Max said. Also, feel free to @ me so In reply faster :)
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.
@pzhanggit @RylieWeaver
I personally found the explanation given by ChatGPT very useful. I attach it here hoping that you will find it useful too.
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.
Yep. Wow, the o1-preview is great
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.