search

JuliaMolSim / Molly.jl

Molecular simulation in Julia
Other
393 stars 53 forks source link

Unit conversion for general interactions #121

Closed swyant closed 1 year ago

swyant commented 1 year ago

Hello,

I'm currently working on improving the interoperability between InteratomicPotentials.jl and Molly. I have a mostly working prototype (based off of some past work) using Lennard-Jones as an example that can be seen here. The near-term goal is to get various machine-learned interatomic potentials working as well.

The primary issue is that the forces outputted from InteratomicPotentials.jl are in atomic units (i.e. Hartree/Bohr), but I typically use units of eV/Å. However, if I set my ForceUnits=u"eV/Å" in my Molly system, it appears that no unit checking occurs (for the general interactions), the units are stripped, and the user-specified units are incorrectly appended, with no unit conversion. While I can manage the units on my end, I think it would be safer/more reliable if these units were converted appropriately within Molly.

I've discussed this issue a bit with @leios already, but any additional help is appreciated! Thank you!

jgreener64 commented 1 year ago

Looks cool, and I am excited about the machine learning potentials.

You are correct that currently forces are stripped without checking, which is a problem: https://github.com/JuliaMolSim/Molly.jl/blob/588e46372fcebbb1d263907812e5ad4e49274a3f/src/force.jl#L298-L301

This is fixed on the work in progress kernels branch though:

https://github.com/JuliaMolSim/Molly.jl/blob/7ea9481346aea78ca933401281219644fd4d62c0/src/force.jl#L167-L169

Hopefully that branch should be merged soon, it contains many other improvements too. I just need to clean up the docs a bit and wait for a new Enzyme release to be tagged.

swyant commented 1 year ago

Thanks! I will start to use the kernels branch in the meantime to continue setting up our code, and will switch back once its merged.

jgreener64 commented 1 year ago

It was merged a few days ago so you should be good to go.

jgreener64 commented 1 year ago

I should add that currently on GPU the units returned by the interaction should be the same as the system force units, being convertible to them isn't sufficient. This is because there was a slight performance hit of allowing the conversion, which may be fixed in future.

swyant commented 1 year ago

Ah, good to know! Thanks