Skip to content
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

[LAMMPS] Full unit-awareness when parsing LAMMPS outputs with drivers #902

Open
timbernat opened this issue Feb 14, 2024 · 3 comments
Open
Labels
lammps Relating to LAMMPS

Comments

@timbernat
Copy link

Description
In the current implementation of get_lammps_energies, log file parsing is hard-coded to energies of kJ/mol and has no awareness of the various unit styles that LAMMPS supports. Currently MDConfig.write_lammps_input() only supports the "real" unit style, so this is a fair assumption for the current implementation, but is not robust in the not-unreasonable case where one is provided/generates an input file which deviates from the MDConfig hard-coded defaults. Dynamic awareness of the unit system being used would remove a source or error (and a lot of guesswork) when comparing energies between platforms, as well as other unit-dependent quantites (e.g. unit cell sizes, velocities, etc)

I've already implemented both a comprehensive reference of LAMMPS unit styles (in OpenMM units) and functionality for detecting/applying these settings from input files. Would these codes (in some modified form) be amenable to a PR or future feature?

@timbernat timbernat changed the title Full unit-awareness when parsing LAMMPS outputs with drivers [LAMMPS] Full unit-awareness when parsing LAMMPS outputs with drivers Feb 14, 2024
@mattwthompson
Copy link
Member

Quick thoughts now, might add more later - happy to look at a PR for any of this!

  • Using LAMMPS's Python wrapper is better than the raw parsing Interchange currently uses
  • All current OpenFF use cases work fine with real and might not work well with other styles, or I don't understand biophysics use cases in which other styles are valuable
  • I'm apprehensive about different unit systems creeping out into the core API or our infrastructure more generally; the behavior within a LAMMPS submodule can be whatever as long as there's some consistency in what's reported out of it (i.e. get_lammps_energies)

@timbernat
Copy link
Author

timbernat commented Feb 14, 2024

  • Using LAMMPS's Python wrapper is better than the raw parsing Interchange currently uses
    I'm in agreement there!
  • All current OpenFF use cases work fine with real and might not work well with other styles, or I don't understand biophysics use cases in which other styles are valuable
    Admittedly neither do I, I mostly included this for the sake of generality to have (if nothing else) an API-compatible reference of the LAMMPS styles
  • I'm apprehensive about different unit systems creeping out into the core API or our infrastructure more generally; the behavior within a LAMMPS submodule can be whatever as long as there's some consistency in what's reported out of it (i.e. get_lammps_energies)
    That makes perfect sense, were I to put together a PR for this, I'd rewrite using OpenFF/pint units (I only wrote in using OpenMM units because that's what I prefer to use day-to-day :) ). An additional wrinkle regardless of the unit engine chosen is that LAMMPS in certain places uses units not defined by default in either openmm.unit or in the Default pint UnitRegistry (e.g. electronvolts, statcoulombs, hertz). I've written my own definitions of these additional units, which would have to be also be done if using the OpenFF unit system.

@mattwthompson
Copy link
Member

One of the text files here controls which are exposed by our registry, which is mostly Pint's default with macro-scale units stripped out. Adding units to the registry is nearly free, so anything that might be useful for your work (eV, etc.) is probably useful for others and can be added

https://github.com/openforcefield/openff-units/tree/cfdb193b6718776a00fde378afb17617d2dfabb0/openff/units/data

@mattwthompson mattwthompson added the lammps Relating to LAMMPS label Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lammps Relating to LAMMPS
Projects
None yet
Development

No branches or pull requests

2 participants