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
Consistent naming between dy and forces #121
Conversation
Do you also want to rename |
Initially I thought torch geometric was requiring setting |
I'm a bit divided here. We use torchmd-net also for pKa prediction and in the future maybe also for charge prediction. This will make it more confusing, will it not? |
Do you mean renaming both |
I think renaming them both is confusing. I think they should be named energy/forces only internally at the datasets level (and they should return y/dy) and not at the train/script level where you could replace the datasets with other type of data. The problem here being that we don't train on dy but on minus_dy which makes it ugly. |
If the models are intended to be used for predicting things other than energy, I agree that |
Changing the model to return the positive gradient now would silently break a lot of applications, I don't think that's a good idea. I'd be okay with calling it |
Regarding the variable names, my preference is |
Another options: |
I updated (hopefully) all places to consistently use |
neg_dy is the best compromise in my view
…On Wed, Aug 31, 2022, 20:52 Philipp Thölke ***@***.***> wrote:
Changing the model to return the positive gradient now would silently
break a lot of applications, I don't think that's a good idea. I'd be okay
with calling it y and neg_dy or something like that to make it clear.
—
Reply to this email directly, view it on GitHub
<#121 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3KUORMOPSLA5VL6FBCYMLV36SWPANCNFSM577VBPCQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
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 ugly, but at least consistent.
This PR renames
y
toenergy
anddy
toforces
in places where we mean forces and not gradients. PyTorch-GeometricData
objects are now constructed asData(energy=<energy>, ...)
orData(energy=<energy>, forces=<forces>, ...)
. This should not change any behavior but just concerns naming conventions. Linked to #116