search

torchmd / torchmd-net

Training neural network potentials
MIT License
335 stars 75 forks source link

Consistent naming between dy and forces #121

Closed PhilippThoelke closed 2 years ago

PhilippThoelke commented 2 years ago

This PR renames y to energy and dy to forces in places where we mean forces and not gradients. PyTorch-Geometric Data objects are now constructed as Data(energy=<energy>, ...) or Data(energy=<energy>, forces=<forces>, ...). This should not change any behavior but just concerns naming conventions. Linked to #116

peastman commented 2 years ago

Do you also want to rename y to energy? It seems like it would be more consistent to either name both as abstract variables (y and dy, in which case the latter would be the gradient of the former), or else name both as physical quantities (energy and forces, in which case it's the negative gradient). Mixing them can be confusing.

PhilippThoelke commented 2 years ago

Initially I thought torch geometric was requiring setting y when constructing a Data object but now I'm not sure anymore. I agree that energy and forces makes more sense than y and forces. I'll rename y as well if torch geometric doesn't prohibit that.

stefdoerr commented 2 years ago

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?

PhilippThoelke commented 2 years ago

Do you mean renaming both y and dy are confusing or would you want dy to be forces and y to stay as it is?

stefdoerr commented 2 years ago

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.

peastman commented 2 years ago

If the models are intended to be used for predicting things other than energy, I agree that y and dy is a better choice, and dy should be the positive gradient.

PhilippThoelke commented 2 years ago

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.

raimis commented 2 years ago

Regarding the variable names, my preference is d/dy, but energy/forces is fine. Could they be made consistent in models.model.TorchMD_Net too.

raimis commented 2 years ago

Another options: models.model.TorchMD_Net.forward does not only the computation of y (the forward pass), but also the computation of dy (the backward pass). We can factor out the gradient computation into a separate function/method. As a side effect, the sign of dy could be changed without breaking the others code silently.

PhilippThoelke commented 2 years ago

I updated (hopefully) all places to consistently use y as the label/prediction and neg_dy as the negative derivative of the label/prediction.

giadefa commented 2 years ago

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 https://github.com/torchmd/torchmd-net/pull/121#issuecomment-1233296689, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3KUORMOPSLA5VL6FBCYMLV36SWPANCNFSM577VBPCQ . You are receiving this because your review was requested.Message ID: @.***>