-
Notifications
You must be signed in to change notification settings - Fork 81
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
Convert all amber ions; add OPC and OPC3 waters #239
Conversation
Thanks so much for contributing this! We're currently on AmberTools 20.15---would it make sense to include this in a full update of all Amber force fields to AmberTools 22? |
@jchodera Welcome! About AmberTools 22 - I'm game to give it a try, but I don't know enough about the conversion process to easily debug any potential issues that pop up if it doesn't work out of the box. If it's okay, perhaps we can do it in two stages - first get the updated water and ions (so we can get OPC, OPC3+ions, and the newer L&M ions for TIP3P-FB and TIP4P-FB in next openmm 8.x release), then update everything else? |
Any idea what has changed between 20 and 22? |
@peastman I don't know exactly what the release notes say, but from a quick diff looks like the main additions are:
What hasn't changed (at all, as far as I can tell)
|
@jchodera I think that's mainly a question for you. I notice the CI builds are failing. It looks like that's due to the lack of an OpenEye license. Is that something that needs to be fixed in the project settings? |
If the license is set up in the way that I'm familiar with, it is by design not accessible from forks and should remain that way, unfortunately. I've pushed these commits to an upstream branch and #242 at least to see if CI passes, though I'm not sure what is tested that would cause CI to fail. I'd be happy to offer help but I'm not comfortable enough with Amber internals to review. Agree John would be the best approver here; you might be better able to get his attention through other channels. |
@aizvorski : Can you take a look at #242 and see if it looks good to merge to supplant this pR? |
Merged as #242 |
This converts one-to-one all Amber ion frcmod files (from AmberTools 22) into OpenMM xml files with the same names and the same contents. Implements what is proposed in openmm/openmm#3663 Moved here based on feedback from @peastman
Run the conversion:
The new xml files end up in
amber/ffxml/ions/
This does not change any of the existing ion conversion, and also does not combine water and ions in a single file. The intention is to have the water+ions be made by using
<Include file="..."/>
mechanism in the xml, rather than by duplicating the contents; but that is a future change, at the moment this doesn't change any existing files. It also doesn't use yaml - since the idea is to convert everything, there is nothing to configure.Also, adds OPC and OPC3 waters, with the appropriate ion sets. The waters are from openmm/openmm#3654 and the opc_standard.xml file shows the proposed method for combining water and ions.
Feedback requested:
validate_water_ion()
has a nice unit test for water+ions, but not obvious how to run that outside of a yaml recipe