shirtsgroup / InterMol

Conversion tool for molecular simulations
MIT License
185 stars 53 forks source link

Lammps parser fix #379

Open orionarcher opened 2 years ago

orionarcher commented 2 years ago

This is meant to address Issue #378, which identifies bugs with the dihedral_style opls and the improper_style cvff.

It uses some code from PR #349, written by @JoshuaSBrown. I rewrote the code here because that PR had not been touched in 2 years.

ctk3b commented 2 years ago

One small comment but if this fixes your examples then this LGTM

mrshirts commented 2 years ago

@ctk3b, if you had a chance to review some of these and accept, since you wrote chunks of it and are now a better coder that me, it might be useful :). No worries if you can't . . .

ctk3b commented 2 years ago

Yup! I don't have time to get LAMMPS up and running locally and confirm this but the change looks correct to me.

If @orioncohen can confirm this works for some LAMMPS files then I would recommend merging this

mrshirts commented 2 years ago

@orioncohen - how about you write the proper tests to verify the new functionality works, and then we get it checked in?

mattwthompson commented 2 years ago

I can take a look at this (I have LAMMPS installed, etc.) if provided a few input files. I can also make a release once this is merged in, if that's more convenient than installing from source.

orionarcher commented 2 years ago

Thanks for the feedback everyone.

I've had a chance to play with this a bit more, and while this has allowed me to read and write my files, but they are being corrupted in the process. The impropers are being removed and the dihedral_style is being incorrectly identified as charmm despite specifying opls in the input.

Whatever the cause, clearly my "fix" is allowing something to happen that should not. I don't have the debugging time to get to the heart of this.

@mattwthompson, if you want to take a look at this I'd be happy to send along some input files.

mattwthompson commented 2 years ago

Could you provide some input files so this can be tested out?

orionarcher commented 2 years ago

Sorry for the delay. I just committed some sample input files.

mattwthompson commented 2 years ago

Thanks - I'll have a look but it might take me about week to get to it 🙂

mattwthompson commented 2 years ago

I can't get that script + data file to run out of the box - does it work on your machine?

(openff-dev) [InterMol] wget https://raw.githubusercontent.com/shirtsgroup/InterMol/1ce4f90493c3bf162dcabd90b75011b218600271/intermol/test_convert_lammps/benzene.lmp & pmd-six  ✭ ✱
[1] 51353

Redirecting output to ‘wget-log’.
[1]  + 51353 done       wget
(openff-dev) [InterMol] wget https://raw.githubusercontent.com/shirtsgroup/InterMol/1ce4f90493c3bf162dcabd90b75011b218600271/intermol/test_convert_lammps/test_input.lammpsin &
[1] 51405

Redirecting output to ‘wget-log.1’.
[1]  + 51405 done       wget
(openff-dev) [InterMol] lmp_serial -i test_input.lammpsin                                                                                                               pmd-six  ✭ ✱
LAMMPS (29 Sep 2021)
Reading data file ...
  orthogonal box = (-2.1778700 0.99881000 -0.94158000) to (47.822130 50.998810 49.058420)
  1 by 1 by 1 MPI processor grid
ERROR: Incorrect args for dihedral coefficients (src/MOLECULE/dihedral_opls.cpp:268)
Last command: read_data        benzene.lmp

I'm lazy and using somebody else's LAMMPS build, but I'm pretty sure it has the necessary packages built with it: https://github.com/conda-forge/lammps-feedstock/blob/4098a282c2ade64a27f243f4a7926acc9dc2b90f/recipe/build.sh#L3

orionarcher commented 2 years ago

The file I sent won't actually run, it is a subset of a valid file that only contains the information that InterMol parses. Sorry that I wasn't clear. I left in only the parts that are relevant to the IO problem at hand, I can send a more complete file if that would be helpful though.

I can't even get that extremely minimal file to be loaded by Intermol. (and I am 99% sure that I included all of the keywords that InterMol searches for).

yel21 commented 2 years ago

Dear developers, Is it any chance to solve the problem with conversion LAMMPS files to GROMACS?