-
Notifications
You must be signed in to change notification settings - Fork 54
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 parser fix #379
base: master
Are you sure you want to change the base?
Lammps parser fix #379
Conversation
….e. if improper_style is cvff
…t file are left out
One small comment but if this fixes your examples then this LGTM |
@@ -175,6 +175,9 @@ def canonical_dihedral(self, params, dihedral, direction='into'): | |||
elif dihedral == FourierDihedral: | |||
convertfunc = convert_dihedral_from_fourier_to_trig | |||
converted_dihedral = TrigDihedral | |||
elif dihedral == TrigDihedral: | |||
convertfunc = lambda x: x |
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.
Could you either remove the converted_dihedral = dihedral # Default
line above or move the convertfunc
to the top as the base case and delete this branch?
@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 . . . |
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 |
@orioncohen - how about you write the proper tests to verify the new functionality works, and then we get it checked in? |
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. |
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. |
Could you provide some input files so this can be tested out? |
Sorry for the delay. I just committed some sample input files. |
Thanks - I'll have a look but it might take me about week to get to it 🙂 |
I can't get that script + data file to run out of the box - does it work on your machine?
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 |
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). |
Dear developers, |
This is meant to address Issue #378, which identifies bugs with the dihedral_style
opls
and the improper_stylecvff
.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.