jparkhill / TensorMol

Tensorflow + Molecules = TensorMol
http://blogs.nd.edu/parkhillgroup
GNU General Public License v3.0
271 stars 75 forks source link

Issue in angular symmetry functions #30

Closed stefdoerr closed 6 years ago

stefdoerr commented 6 years ago

You are calculating the angle of the element pairs using acos https://github.com/jparkhill/TensorMol/blob/master/TensorMol/TFDescriptors/RawSymFunc.py#L175

Acos returns values in [0, 180] degrees. The angular symmetry functions defined by ANI1 are periodic over 360 degrees but not over 180 degrees. So the gradients of the symmetry functions you use are discontinuous at 180 where your angles wrap around.

Look at the symmetry functions here: image

Now imagine you mirror this plot at 180 (which is what acos does). The red symmetry function goes down reaching 180 and then suddenly it goes up again with a discontinuity.

We just found out about this issue in our network so I thought I should inform you as well since you are doing the same thing here.

As a proof, this is what happens to the forces our network produces if you rotate one hydrogen of a water in 360 degrees.

figure_1-11

Just thought you might be interested since in ANI1 they seem to not have that problem so I assume they use atan for the angles to be in the range [-pi, pi]

Best, Stefan

jeherr commented 6 years ago

@stefdoerr I think you are correct here. This certainly needs fixed. I believe that it should actually use atan2 rather than atan to get the correct angles. Then the thetas should range from -pi to pi rather than 0 to 2pi. I could fix this before too long, but if you have it fixed feel free to make a pull request.

stefdoerr commented 6 years ago

No I am still thinking about it because you cannot really determine the sign of an angle uniquely in a 3d case. You need some frame of reference. I found this: https://stackoverflow.com/a/33920320/1198173 but I have not yet gotten around to testing it

jeherr commented 6 years ago

Yes this solution uses atan2 as I suggested instead of atan. The reason it works is because it keeps track of the sign of both cos(theta) and sin(theta) when figuring out the sign of the resulting angle. The second answer to this stackoverflow question gives information in regards to this difference between atan and atan2.

The problem with a frame of reference is that you would lose the rotational invariance of the symmetry functions.

stefdoerr commented 6 years ago

The alternative is to use arccos and scale the symmetry functions to be symmetrical in the 0-pi range. Might make more sense actually considering the lack of a unique frame

On April 27, 2018 8:16:14 PM GMT+02:00, "John E. Herr" notifications@github.com wrote:

Yes this solution uses atan2 as I suggested instead of atan. The reason it works is because it keeps track of the sign of both cos(theta) and sin(theta) when figuring out the sign of the resulting angle. The second answer to this stackoverflow question gives information in regards to this difference between atan and atan2.

The problem with a frame of reference is that you would lose the rotational invariance of the symmetry functions.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/jparkhill/TensorMol/issues/30#issuecomment-385052265

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

jparkhill commented 6 years ago

Stefan - Would you like to be added to the package authors list? Best- John

giadefa commented 6 years ago

Hi John, just to clarify we are using our own code not tensormol. I suggested Stefan to send you the tip as we saw that you had the same mistake as us.

g

On Fri, Apr 27, 2018 at 9:06 PM, John Parkhill notifications@github.com wrote:

Stefan - Would you like to be added to the package authors list? Best- John

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jparkhill/TensorMol/issues/30#issuecomment-385065809, or mute the thread https://github.com/notifications/unsubscribe-auth/AHaqOps5vdaLRd_zDjF_FGaDA-zXdtUjks5ts2xJgaJpZM4Tqqv3 .

giadefa commented 6 years ago

We are happy to collaborate though if there is the opportunity in the future.

On Fri, Apr 27, 2018 at 9:23 PM, Gianni De Fabritiis < g.defabritiis@gmail.com> wrote:

Hi John, just to clarify we are using our own code not tensormol. I suggested Stefan to send you the tip as we saw that you had the same mistake as us.

g

On Fri, Apr 27, 2018 at 9:06 PM, John Parkhill notifications@github.com wrote:

Stefan - Would you like to be added to the package authors list? Best- John

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jparkhill/TensorMol/issues/30#issuecomment-385065809, or mute the thread https://github.com/notifications/unsubscribe-auth/AHaqOps5vdaLRd_zDjF_FGaDA-zXdtUjks5ts2xJgaJpZM4Tqqv3 .

jparkhill commented 6 years ago

Yes -- That was clear guys. I just extended the offer because it was just an example of an intellectual contribution, although the stuff we are currently developing does not use the ANI symmetry functions or BP style network.

Gianni - Actually we should talk. I think a lot of the things we are trying to do are pretty complementary. htmd is the cleanest/best version of what it is...

giadefa commented 6 years ago

sure, I'll email you.

g

On Fri, Apr 27, 2018 at 10:02 PM, John Parkhill notifications@github.com wrote:

Yes -- That was clear guys. I just extended the offer because it was just an example of an intellectual contribution, although the stuff we are currently developing does not use the ANI symmetry functions or BP style network.

Gianni - Actually we should talk. I think a lot of the things we are trying to do are pretty complementary. htmd is the cleanest/best version of what it is...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jparkhill/TensorMol/issues/30#issuecomment-385079075, or mute the thread https://github.com/notifications/unsubscribe-auth/AHaqOkIgyvCnNL7TX20-gf_Es_oGt5x0ks5ts3k7gaJpZM4Tqqv3 .

stefdoerr commented 6 years ago

So on the subject of this issue as we discussed with @jeherr, having signed angles would probably mess up the rotational symmetry as it requires a frame of reference and depending on the order of atoms would produce either positive or negative angle for the same molecule. So I went now the alternative way of keeping acos but using cos(2*(theta_ijk - theta_s)) instead of cos(theta_ijk - theta_s) in the angular symmetry functions so that they are symmetrical in 0 to pi and moved the centers of the functions to be in the range 0 to pi. I will train now over the weekend and hopefully have some good news on Monday :) I kept the same number of angular functions (8) so theoretically it should have an even better resolution of angles. I might consider cutting them down to 4 later to reduce the number of parameters by half

jparkhill commented 6 years ago

Yes. Your PES angular scan looked pretty noisy (besides the artifact at 180) Besides just reducing # of parameters, you should consider generating training samples by biasing against samples whose ANI-SF's overlap strongly with your training set. Also ensure you're using an infinitely differentiable neuron, and at least twice differentiable cutoffs.

jeherr commented 6 years ago

Closing as this will be fixed in a future version of TensorMol. Thanks for the help Stefan and Gianni.