-
Notifications
You must be signed in to change notification settings - Fork 16
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
LammpsPotential: adapt for using Nequip potentials #118
base: master
Are you sure you want to change the base?
Conversation
The current implementation cannot exploit other potentials, such as neural network potentials, in this case, coming from Nequip architecture. The implementation now is extended to be able to use this ML family of potentials.
Hi, I wanted to implement these modifications to be able to run LAMMPS using NN-ML from Nequip architectures. They are still to refine, so happy to have your feedbacks. |
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #118 +/- ##
=======================================
Coverage 85.43% 85.44%
=======================================
Files 19 19
Lines 1614 1615 +1
=======================================
+ Hits 1379 1380 +1
Misses 235 235
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
A couple of questions
- Why do you need to change the base extension of the potential file?
- Why do you need to change how the file is being read? Is there any reason why you need to do it in this way? Would this affect other kinds of potentials? I do not think so but I just wonder if you have tested this.
- Why do you need to pass the potential file name to the input file generator?
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.
- This is needed as the potentials used by Nequip/Allegro are written in Pytorch, and I believe it wouldn't work otherwise with .dat extension.
- As it is a .pth file, then one needs to specify the binary format for the reading.
- The reason being otherwise it would write it as .dat, whereas one needs to specify .pth
These changes are needed when one patches LAMMPS with pair_nequip/pair_allegro potentials. Of course, maybe we want to make these decision automatically, e.g. by checking the name of the potential (in these cases either nequip or allegro), and then use the corresponding filename and reading format.
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.
- That is okay, I was mostly curious, I just wonder why the extension matters but if it does I se no harm in changing it. A question are these potentials created before hand by the program?
- Okay so it is a binary format, but in the current implementation one stores the contents of the file in a
SinglefileData
orm. That is why my confusion, do you need the two handlers?
These changes are needed when one patches LAMMPS with pair_nequip/pair_allegro potentials. Of course, maybe we want to make these decision automatically, e.g. by checking the name of the potential (in these cases either nequip or allegro), and then use the corresponding filename and reading format.
I'd just like to keep that the potential class handles all the potentials in the same way if it is possible. I'm not familiar with these potentials, but for what I understand these are binary files that LAMMPS reads, which maybe are created before hand? Or maybe they are updated on the fly by some active learning approach? In either case, what would be preferable is that one can treat these potentials in the same way that other types to reduce the complexity of the plugin.
Hi @bastonero nice to have your inputs, I left a couple of messages in your code about some changes that I do not fully understand why they are necessary, |
I notice also that the tests are failing and this seems to be a general issue after the release, I've been very busy the last weeks, but I will try to look into this. |
Hi @bastonero I have made a new release which should take care of the failing tests. Feel free to try it out. |
860b346
to
e55a8e5
Compare
The current implementation cannot exploit other potentials, such as neural network potentials, in this case, coming from Nequip architecture. The implementation now is extended to be able to use this ML family of potentials.