-
Notifications
You must be signed in to change notification settings - Fork 63
LAMMPS Calculator #213
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
base: master
Are you sure you want to change the base?
LAMMPS Calculator #213
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #213 +/- ##
===========================================
- Coverage 69.55% 22.15% -47.40%
===========================================
Files 41 42 +1
Lines 6884 6938 +54
===========================================
- Hits 4788 1537 -3251
- Misses 2096 5401 +3305 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looks interesting. Could potentially be an extension in LAMMPS.jl if you wanted to maintain it yourself. |
|
Yeah I decided its just easier to have it here lol. I can live with downloading Molly. I'll add some tests for this soon. The energies do appear to be correct, but I'm having some issues with multi-threading. |
|
@jgreener64 This is ready (save for documentation). I ran the tests manually and they work, but I'm not really sure how to setup tests for a package extension? The |
|
On balance I think this might be better off as an extension in LAMMPS.jl. Extensions are available for testing assuming the required packages are imported. I didn't get round to setting up the ASE tests on CI. |
|
Yeah I can ask them if they'd be ok adding this there. I do still thing this interoperability should be advertised in Molly somewhere. Cause to be honest if I can't use LAMMPS through Molly, then I would never consider Molly for my research. |
|
Ah I thought LAMMPS.jl was your project, which was one of the reasons for suggesting it go there. Maybe it could go in Molly, I'll take a look. |
|
I also asked them over at LAMMPS.jl as well. In principle this could be a separate package, I just don't want it lost as I think its very useful. |
|
I agree it is useful and we can document it here either way. |
|
Having had a look it seems that this is mostly independent of Molly, so it might get more use as its own package that Molly can import the calculator from. We can still have an example in the Molly docs. I would hesitate to use If you wanted to keep the sanity checks for Molly systems, you could have a Molly extension in the package that overloads some function |
|
Just hopping to comment here. IMO this would be the best to implement in LAMMPS.jl and adding full AtomsCalculators support as an extension. This way everyone would get to use it not just Molly. |
|
Yea I can discuss with them more. I did hit a technical issue though. I dont think you can call LAMMPS without it rebuilding a neighbor list every time. This makes it like 20-50x slower than it otherwise would be just getting forces. If anyone knows a way around that I'd love to hear it. Right now I basically just set positions and call "run 0". |
|
I forgot to ask have you tried IPICalculator.jl with LAMMPS? That would the easiest way to use LAMMPS in Molly. It might be a good idea to add a documentation page on Molly on how to use i-PI interface. |
A new package extension for LAMMPS.jl that defines a general interaction which uses LAMMPS to calculate force and energies. It even uses OpenMP to do multi-threaded calculations!
This requires a new tagged version of LAMMPS.jl before we can merge as I made some PRs there to enable in-place calculation of forces.
In principle this approach could be used to have LAMMPS calculate properties of the system as well.