-
Notifications
You must be signed in to change notification settings - Fork 76
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
Foyer should identify missing parameters when it fails #323
Comments
Replaced the vapor_box_param assignment with this, but did not get particularly useful output. UnboundLocalError Traceback (most recent call last) ~/anaconda3/lib/python3.7/site-packages/foyer/forcefield.py in apply(self, topology, references_file, use_residue_map, assert_bond_params, assert_angle_params, assert_dihedral_params, assert_improper_params, combining_rule, verbose, *args, **kwargs) ~/anaconda3/lib/python3.7/site-packages/foyer/forcefield.py in parametrize_system(self, topology, positions, references_file, assert_bond_params, assert_angle_params, assert_dihedral_params, assert_improper_params, combining_rule, verbose, *args, **kwargs) ~/anaconda3/lib/python3.7/site-packages/foyer/forcefield.py in _check_dihedrals(data, structure, verbose, assert_dihedral_params, assert_improper_params) UnboundLocalError: local variable 'pmd_ids' referenced before assignment |
Try |
Still getting the same output related to "UnboundLocalError" |
Matt shared your XML and I think I found the issue. In this line:
I think there is a typo at |
I fixed the the "phase" problem, but the end result is the same for n-butane. |
@jpotoff What version of foyer are you using? And how did you install? I am able to type a butane in GAFF (periodic dihedrals) with |
@rsdefever where is your GAFF xml hiding? I saw it recently, but can't seem to find it. |
I am running foyer 0.7.3 |
Using the original XML and this snippet (modify the SMILES string): ff=foyer.Forcefield(forcefield_files='/Users/mwt/Downloads/example/opls_gomc.xml')
ff.apply(mb.load('CCCC', smiles=True)) ethane and propane can be typed, but butane cannot be |
The issue is when we check the dihedrals, as In this case, the structure identifies 27 dihedrals (I think this is correct). In this case, the C-C-C-C has 3 dihedrals for each periodicity listed in the XML (I think this is fine but I'm not sure). Because of these 3 dihedrals, there are a total of 29 proper_dihedrals in the system. I think this is a bug (I can raise this as an issue), but for now passing |
I don't think thats the issue that @jpotoff is running into. I tried For me with GAFF, yes, I need to use GAFF XML here: https://github.com/rsdefever/antefoyer If you just want the XML file: https://github.com/rsdefever/antefoyer/blob/master/antefoyer/xml/gaff.xml |
To make sure we're on the same page, this is the xml file I'm currently using: https://gist.github.com/rmatsum836/9e6ceee6cddc5ac059453ee3ce368bd4 And this is the script I'm using: https://gist.github.com/rmatsum836/94070e302d40cde8974de4697b0accfb |
Ok, so loaded up gaff.xml using And I'm still getting the same error... |
Make the above change to your xml ff=foyer.Forcefield(forcefield_files='gaff.xml')
vapor_box_param=ff.apply(vapor_box, assert_dihedral_params=False, verbose=False)
In a perfect world, the dihedral checking would do a better job looking at periodic torsions and rb torsions, and it would also do a better job checking that every dihedral tuple has a dihedraltype, not just the numbers of dihedral types match the number of dihedral tuples |
Using these commands I was able to get foyer to output parameters: Using my opls_gomc.xml file also worked. But...this behavior from foyer needs to be fixed. Turning on a "verbose" flag to get more information should never break the code. Simply checking that the number dihedral types matches the number of dihedral bonded tuples is not the best practice as it only works as intended for parameter files that use RB torsions. Furthermore, the default behavior of foyer does not follow what is given in the documentation: https://mosdef.org/foyer/atom-typing_options.html
In this case, all dihedrals were defined, but foyer exited with an error (i.e. the opposite behavior is produced). Dihedral checking had to be turned off to write the parameter file (via assert_dihedral_params=False). I see @rsdefever raised this in issue #295. Running with assert_dihedral_params=False is, quite frankly, dangerous. I could delete some of the torsions from the XML parameter file and foyer was happy to produce a parameter file minus the torsions I has deleted. Only a "warning" is produced instead of failing with an error. At no point does foyer ever tell me what torsions are missing. Overall, this is not appropriate behavior for a tool that one expects will be used by non-experts. Two things are going to happen. 1) users will get frustrated and give up, or 2) they disable safety checks just to get foyer to produce some kind of output, and run bad simulations. Fortunately, those missing torsions will be caught by codes like NAMD, GOMC, GROMACS, etc, but other codes may not. |
There are several things happening here, so I am going to try to break them down into distinct issues that we can address (in order of easiest to hardest, I think).
Which, is fine from the perspective of defining the potential, but challenging for us because it is hard to know that the dihedral was found, but had a force constant of 0.0. This may be something that is easier to address once we move over to GMSO for applying forcefield parameters. @jpotoff I agree that this is a problem. It makes it difficult to be sure that all of the dihedral were indeed parameterized. If others agree with this breakdown and commentary, I would advocate for closing this issue and opening new discrete issues for each of these so we can tackle them. |
I agree, and is something I can address in a PR. The fix won't be perfect, but we can at least prevent
I think checking dihedrals will become easier once we replace Parmed with GMSO. A big headache right now is that RB dihedrals and periodic dihedrals are stored as separate attributes in a parmed structure. In GMSO we will keep track of all proper dihedrals in a single attribute which should make the book keeping easier. |
When Foyer fails to assign parameters (especially for dihedrals), it should identify the parameter types that it could not find in the XML file.
example.zip
The text was updated successfully, but these errors were encountered: